Add extended attribute, [Measure], to simplify use counting

Add extended attribute, [Measure], to simplify use counting

Setting [Measure] applies use counting to that member.
Use counter references are generated based on name with a type suffix,
for example:

interface Test {
  [Measure]  attribute double thing;
  [Measure]  void action();
}

will reference UseCounter::V8Test_Thing_AttributeGetter, UseCounter::V8Test_Thing_AttributeSetter,
and UseCounter::V8Test_Action_Method.

Note that this also allows getters/setters to be counted separately which
previously required custom bindings.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191253

[dstockwell, 2015-01-26 02:57:24]

dstockwell@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-01-26 03:44:59]

haraken@chromium.org changed reviewers:
+ bashi@chromium.org, jl@opera.com

[haraken, 2015-01-26 03:44:59]

+bashi, +jl

> [MeasureAs=] and [DoNotMeasure] will override the behaviour of [Measure] to
> rename or disable use counting.

Hmm, this sounds a bit complicated to me. On what interfaces do you want to use
[Measure]? (Is it too verbose to add [MeasureAs] to each attribute/method?)

[dstockwell, 2015-01-26 03:54:52]

On 2015/01/26 at 03:44:59, haraken wrote:
> +bashi, +jl
> 
> > [MeasureAs=] and [DoNotMeasure] will override the behaviour of [Measure] to
> > rename or disable use counting.
> 
> Hmm, this sounds a bit complicated to me. On what interfaces do you want to
use [Measure]? (Is it too verbose to add [MeasureAs] to each attribute/method?)

I plan to use this on all core/animation interfaces, and (see below) eventually
all interfaces.

I think we should consider to prefer Measure over MeasureAs for consistent
naming. Longer term we should make Measure the default, and require DoNotMeasure
to be specified for performance critical interfaces. Historically we have
measured bindings only for the purpose of deprecation. But often we would have
benefited from having data earlier (to make such deprecation choices) or to
understand the uptake of new features. Measurement is cheap, we should encourage
measuring all new interfaces.

[haraken, 2015-01-26 05:47:49]

On 2015/01/26 03:54:52, dstockwell wrote:
> On 2015/01/26 at 03:44:59, haraken wrote:
> > +bashi, +jl
> > 
> > > [MeasureAs=] and [DoNotMeasure] will override the behaviour of [Measure]
to
> > > rename or disable use counting.
> > 
> > Hmm, this sounds a bit complicated to me. On what interfaces do you want to
> use [Measure]? (Is it too verbose to add [MeasureAs] to each
attribute/method?)
> 
> I plan to use this on all core/animation interfaces, and (see below)
eventually
> all interfaces.
> 
> I think we should consider to prefer Measure over MeasureAs for consistent
> naming. Longer term we should make Measure the default, and require
DoNotMeasure
> to be specified for performance critical interfaces. Historically we have
> measured bindings only for the purpose of deprecation. But often we would have
> benefited from having data earlier (to make such deprecation choices) or to
> understand the uptake of new features. Measurement is cheap, we should
encourage
> measuring all new interfaces.

Hmm... I guess that the Measurement is not cheap and there would be a lot of DOM
attributes/methods on which we should not add the measurement mechanism
implicitly.

I'd like to hear an opinion of Jens.

[Jens Widell, 2015-01-26 07:14:48]

As a way of conveniently measuring everything in an interface this makes sense.
It adds an acceptable amount of complexity to the system, I think. And
consistent (and effortless) naming is nice.

Whether it actually makes sense to measure everything in an interface is a
different question altogether. I'm very skeptical about applying this broadly.
Each measuring instance is cheap, but if we measure all methods and attributes
the cost will add up, and it's not like Blink is already too fast and too small
(binary footprint).

Anyway, that's a discussion for blink-dev@, I think.

https://codereview.chromium.org/877523004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/877523004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:378:
UseCounter::countIfNotPrivateScript(info.GetIsolate(),
callingExecutionContext(info.GetIsolate()),
UseCounter::{{method.measure_as('Overload%d' % method.overload_index)}});
This seems somewhat problematic to me; what a method is measured as depends
implicitly on otherwise irrelevant details in the IDL file, like order, and may
change if the order is changed or if overloads are added or removed. It'd be
difficult to keep track of what each counter actually means over time.

I think I'd prefer if overloads were simply measured as "Method" unless you put
specific [MeasureAs] attributes (with actual names describing the overload,
instead of indexes) on the different method definitions.

[dstockwell, 2015-01-26 07:26:48]

On 2015/01/26 at 07:14:48, jl wrote:
> As a way of conveniently measuring everything in an interface this makes
sense. It adds an acceptable amount of complexity to the system, I think. And
consistent (and effortless) naming is nice.
> 
> Whether it actually makes sense to measure everything in an interface is a
different question altogether. I'm very skeptical about applying this broadly.
Each measuring instance is cheap, but if we measure all methods and attributes
the cost will add up, and it's not like Blink is already too fast and too small
(binary footprint).
> 
> Anyway, that's a discussion for blink-dev@, I think.

Certainly, I will gather data and raise it there at some point.

https://codereview.chromium.org/877523004/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/877523004/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:378:
UseCounter::countIfNotPrivateScript(info.GetIsolate(),
callingExecutionContext(info.GetIsolate()),
UseCounter::{{method.measure_as('Overload%d' % method.overload_index)}});
On 2015/01/26 07:14:48, Jens Widell wrote:
> This seems somewhat problematic to me; what a method is measured as depends
> implicitly on otherwise irrelevant details in the IDL file, like order, and
may
> change if the order is changed or if overloads are added or removed. It'd be
> difficult to keep track of what each counter actually means over time.
> 
> I think I'd prefer if overloads were simply measured as "Method" unless you
put
> specific [MeasureAs] attributes (with actual names describing the overload,
> instead of indexes) on the different method definitions.

That seems a reasonable solution for overloads. Changed to that behavior.

[dstockwell, 2015-01-27 07:30:28]

ping

[Jens Widell, 2015-01-28 12:48:32]

LGTM

Implementation is good, and I can see reasonable uses. I'd be against applying
this overly broadly on whole interfaces (as mentioned earlier) but that doesn't
block landing the implementation.

https://codereview.chromium.org/877523004/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/877523004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:381: measure_as_name = '%s_%s' %
(capitalize(interface.name), measure_as_name)
Nit: I don't know if it makes much sense to use capitalize() on interface names;
they're typically capitalized already.

[haraken, 2015-01-28 13:02:22]

Hmm, I'm still skeptical about this change (Sorry about the late feedback!).

- From the perspective of performance, [Measure] should not be overly used. The
best way to avoid the overuse would be just not to introduce [Measure]. The
current [MeasureAs] model will prevent the overuse.

- It's common that people change attribute/method names when the feature is
under a runtime flag. If they use [Measure], they will end up resetting and thus
misreading the counters unless they take a special care when renaming the
attributes/methods. The current [MeasureAs] model will prevent such a confusing
scenario.

https://codereview.chromium.org/877523004/diff/20001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_utilities.py (right):

https://codereview.chromium.org/877523004/diff/20001/Source/bindings/scripts/...
Source/bindings/scripts/v8_utilities.py:370: def
measure_as(definition_or_member, interface=None):

Can we force the interface parameter? I'm wondering when we want to make it
None.

[dstockwell, 2015-01-28 13:50:37]

On 2015/01/28 at 13:02:22, haraken wrote:
> Hmm, I'm still skeptical about this change (Sorry about the late feedback!).
> 
> - From the perspective of performance, [Measure] should not be overly used.
The best way to avoid the overuse would be just not to introduce [Measure]. The
current [MeasureAs] model will prevent the overuse.

Would you be happier if I do not make [Measure] inherit from interface to
member? That is, you would need to specify [Measure] on every member. I suppose
I can remove [DoNotMeasure] in that case.

> - It's common that people change attribute/method names when the feature is
under a runtime flag. If they use [Measure], they will end up resetting and thus
misreading the counters unless they take a special care when renaming the
attributes/methods. The current [MeasureAs] model will prevent such a confusing
scenario.

It may not be obvious from this patch, but this doesn't remove the need to add
or update enum values in UseCounter.h

>
https://codereview.chromium.org/877523004/diff/20001/Source/bindings/scripts/...
> File Source/bindings/scripts/v8_utilities.py (right):
> 
>
https://codereview.chromium.org/877523004/diff/20001/Source/bindings/scripts/...
> Source/bindings/scripts/v8_utilities.py:370: def
measure_as(definition_or_member, interface=None):
> 
> Can we force the interface parameter? I'm wondering when we want to make it
None.

[haraken, 2015-01-28 15:01:06]

On 2015/01/28 13:50:37, dstockwell wrote:
> On 2015/01/28 at 13:02:22, haraken wrote:
> > Hmm, I'm still skeptical about this change (Sorry about the late feedback!).
> > 
> > - From the perspective of performance, [Measure] should not be overly used.
> The best way to avoid the overuse would be just not to introduce [Measure].
The
> current [MeasureAs] model will prevent the overuse.
> 
> Would you be happier if I do not make [Measure] inherit from interface to
> member? That is, you would need to specify [Measure] on every member. I
suppose
> I can remove [DoNotMeasure] in that case.

Yeah, the implicit inheritance is my concern.

Just to confirm, but then (i.e., if you don't inherit [Measure]) the benefit of
this CL is that you can distinguish getter counters from setter counters, right?

> > - It's common that people change attribute/method names when the feature is
> under a runtime flag. If they use [Measure], they will end up resetting and
thus
> misreading the counters unless they take a special care when renaming the
> attributes/methods. The current [MeasureAs] model will prevent such a
confusing
> scenario.
> 
> It may not be obvious from this patch, but this doesn't remove the need to add
> or update enum values in UseCounter.h

My concern is as follows:

// Before
[Measure] interface Foo {
  void bar();
};

// After
[Measure] interface Foo {
  [MeasureAs=barMethod] void barRenamed();  // If you don't add
[MeasureAs=barMethod], the counter value is not taken over the rename.
};



> 
> >
>
https://codereview.chromium.org/877523004/diff/20001/Source/bindings/scripts/...
> > File Source/bindings/scripts/v8_utilities.py (right):
> > 
> >
>
https://codereview.chromium.org/877523004/diff/20001/Source/bindings/scripts/...
> > Source/bindings/scripts/v8_utilities.py:370: def
> measure_as(definition_or_member, interface=None):
> > 
> > Can we force the interface parameter? I'm wondering when we want to make it
> None.

[dstockwell, 2015-01-30 01:35:42]

On 2015/01/28 at 15:01:06, haraken wrote:
> On 2015/01/28 13:50:37, dstockwell wrote:
> > On 2015/01/28 at 13:02:22, haraken wrote:
> > > Hmm, I'm still skeptical about this change (Sorry about the late
feedback!).
> > > 
> > > - From the perspective of performance, [Measure] should not be overly
used.
> > The best way to avoid the overuse would be just not to introduce [Measure].
The
> > current [MeasureAs] model will prevent the overuse.
> > 
> > Would you be happier if I do not make [Measure] inherit from interface to
> > member? That is, you would need to specify [Measure] on every member. I
suppose
> > I can remove [DoNotMeasure] in that case.
> 
> Yeah, the implicit inheritance is my concern.
> 
> Just to confirm, but then (i.e., if you don't inherit [Measure]) the benefit
of this CL is that you can distinguish getter counters from setter counters,
right?

Yes, also to encourage consistent naming.

> > > - It's common that people change attribute/method names when the feature
is
> > under a runtime flag. If they use [Measure], they will end up resetting and
thus
> > misreading the counters unless they take a special care when renaming the
> > attributes/methods. The current [MeasureAs] model will prevent such a
confusing
> > scenario.
> > 
> > It may not be obvious from this patch, but this doesn't remove the need to
add
> > or update enum values in UseCounter.h
> 
> My concern is as follows:
> 
> // Before
> [Measure] interface Foo {
>   void bar();
> };
> 
> // After
> [Measure] interface Foo {
>   [MeasureAs=barMethod] void barRenamed();  // If you don't add
[MeasureAs=barMethod], the counter value is not taken over the rename.
> };

If you do not add [MeasureAs=] it will not compile, as you will be required to
add V8Foo_BarRenamed_Method to UseCounter.h or rename the existing enum entry. I
think it is desirable to rename the enum values in this case.

[dstockwell, 2015-03-02 22:45:01]

On 2015/01/30 at 01:35:42, dstockwell wrote:
> On 2015/01/28 at 15:01:06, haraken wrote:
> > On 2015/01/28 13:50:37, dstockwell wrote:
> > > On 2015/01/28 at 13:02:22, haraken wrote:
> > > > Hmm, I'm still skeptical about this change (Sorry about the late
feedback!).
> > > > 
> > > > - From the perspective of performance, [Measure] should not be overly
used.
> > > The best way to avoid the overuse would be just not to introduce
[Measure]. The
> > > current [MeasureAs] model will prevent the overuse.
> > > 
> > > Would you be happier if I do not make [Measure] inherit from interface to
> > > member? That is, you would need to specify [Measure] on every member. I
suppose
> > > I can remove [DoNotMeasure] in that case.
> > 
> > Yeah, the implicit inheritance is my concern.
> > 
> > Just to confirm, but then (i.e., if you don't inherit [Measure]) the benefit
of this CL is that you can distinguish getter counters from setter counters,
right?
> 
> Yes, also to encourage consistent naming.
> 
> > > > - It's common that people change attribute/method names when the feature
is
> > > under a runtime flag. If they use [Measure], they will end up resetting
and thus
> > > misreading the counters unless they take a special care when renaming the
> > > attributes/methods. The current [MeasureAs] model will prevent such a
confusing
> > > scenario.
> > > 
> > > It may not be obvious from this patch, but this doesn't remove the need to
add
> > > or update enum values in UseCounter.h
> > 
> > My concern is as follows:
> > 
> > // Before
> > [Measure] interface Foo {
> >   void bar();
> > };
> > 
> > // After
> > [Measure] interface Foo {
> >   [MeasureAs=barMethod] void barRenamed();  // If you don't add
[MeasureAs=barMethod], the counter value is not taken over the rename.
> > };
> 
> If you do not add [MeasureAs=] it will not compile, as you will be required to
add V8Foo_BarRenamed_Method to UseCounter.h or rename the existing enum entry. I
think it is desirable to rename the enum values in this case.

Oops, this patch is still pending. @haraken?

[haraken, 2015-03-03 04:57:55]

Sorry about the late review.

> > > - From the perspective of performance, [Measure] should not be overly
used.
> > The best way to avoid the overuse would be just not to introduce [Measure].
> The
> > current [MeasureAs] model will prevent the overuse.
> > 
> > Would you be happier if I do not make [Measure] inherit from interface to
> > member? That is, you would need to specify [Measure] on every member. I
> suppose
> > I can remove [DoNotMeasure] in that case.
> 
> Yeah, the implicit inheritance is my concern.
> 
> Just to confirm, but then (i.e., if you don't inherit [Measure]) the benefit
of
> this CL is that you can distinguish getter counters from setter counters,
right?

^^^ I want to resolve the above concern. In other words, don't make [Measure]
inherit from an interface to members and remove [DoNotMeasure].

[haraken, 2015-03-03 04:58:52]

On 2015/03/03 04:57:55, haraken wrote:
> Sorry about the late review.
> 
> > > > - From the perspective of performance, [Measure] should not be overly
> used.
> > > The best way to avoid the overuse would be just not to introduce
[Measure].
> > The
> > > current [MeasureAs] model will prevent the overuse.
> > > 
> > > Would you be happier if I do not make [Measure] inherit from interface to
> > > member? That is, you would need to specify [Measure] on every member. I
> > suppose
> > > I can remove [DoNotMeasure] in that case.
> > 
> > Yeah, the implicit inheritance is my concern.
> > 
> > Just to confirm, but then (i.e., if you don't inherit [Measure]) the benefit
> of
> > this CL is that you can distinguish getter counters from setter counters,
> right?
> 
> ^^^ I want to resolve the above concern. In other words, don't make [Measure]
> inherit from an interface to members and remove [DoNotMeasure].

Also I want to confirm the above understanding: If we go that way, the benefit
of this CL is that you can distinguish getter counters from setter counters,
right?

[dstockwell, 2015-03-03 21:23:05]

On 2015/03/03 at 04:58:52, haraken wrote:
> On 2015/03/03 04:57:55, haraken wrote:
> > Sorry about the late review.
> > 
> > > > > - From the perspective of performance, [Measure] should not be overly
> > used.
> > > > The best way to avoid the overuse would be just not to introduce
[Measure].
> > > The
> > > > current [MeasureAs] model will prevent the overuse.
> > > > 
> > > > Would you be happier if I do not make [Measure] inherit from interface
to
> > > > member? That is, you would need to specify [Measure] on every member. I
> > > suppose
> > > > I can remove [DoNotMeasure] in that case.
> > > 
> > > Yeah, the implicit inheritance is my concern.
> > > 
> > > Just to confirm, but then (i.e., if you don't inherit [Measure]) the
benefit
> > of
> > > this CL is that you can distinguish getter counters from setter counters,
> > right?
> > 
> > ^^^ I want to resolve the above concern. In other words, don't make
[Measure]
> > inherit from an interface to members and remove [DoNotMeasure].

OK, I will make this change.

> Also I want to confirm the above understanding: If we go that way, the benefit
of this CL is that you can distinguish getter counters from setter counters,
right?

Yes, I replied earlier: Yes, also to encourage consistent naming.

[dstockwell, 2015-03-03 21:51:49]

> > > ^^^ I want to resolve the above concern. In other words, don't make
[Measure]
> > > inherit from an interface to members and remove [DoNotMeasure].
> 
> OK, I will make this change.

Done. PTAL.

[haraken, 2015-03-04 01:58:39]

Mostly looks good.

Sorry for nit-picking: Do we need both [Measure] and [MeasureAs=*]? Can we treat
[MeasureAs] (without any '=...') as [Measure]? The rule here is "the engine will
decide the counter name if you didn't provide an explicit name".

[dstockwell, 2015-03-04 02:09:55]

On 2015/03/04 at 01:58:39, haraken wrote:
> Mostly looks good.
> 
> Sorry for nit-picking: Do we need both [Measure] and [MeasureAs=*]? Can we
treat [MeasureAs] (without any '=...') as [Measure]? The rule here is "the
engine will decide the counter name if you didn't provide an explicit name".

Right, it would make sense to use [Measure] and [Measure=]. But, [MeasureAs]
doesn't ready correctly, we could do this in a follow on patch.

[haraken, 2015-03-04 02:23:09]

On 2015/03/04 02:09:55, dstockwell wrote:
> On 2015/03/04 at 01:58:39, haraken wrote:
> > Mostly looks good.
> > 
> > Sorry for nit-picking: Do we need both [Measure] and [MeasureAs=*]? Can we
> treat [MeasureAs] (without any '=...') as [Measure]? The rule here is "the
> engine will decide the counter name if you didn't provide an explicit name".
> 
> Right, it would make sense to use [Measure] and [Measure=]. But, [MeasureAs]
> doesn't ready correctly, we could do this in a follow on patch.

Sounds reasonable. LGTM.

[haraken, 2015-03-04 02:23:24]

On 2015/03/04 02:23:09, haraken wrote:
> On 2015/03/04 02:09:55, dstockwell wrote:
> > On 2015/03/04 at 01:58:39, haraken wrote:
> > > Mostly looks good.
> > > 
> > > Sorry for nit-picking: Do we need both [Measure] and [MeasureAs=*]? Can we
> > treat [MeasureAs] (without any '=...') as [Measure]? The rule here is "the
> > engine will decide the counter name if you didn't provide an explicit name".
> > 
> > Right, it would make sense to use [Measure] and [Measure=]. But, [MeasureAs]
> > doesn't ready correctly, we could do this in a follow on patch.
> 
> Sounds reasonable. LGTM.

Sorry about the delay in this review.

[dstockwell, 2015-03-04 05:16:43]

The CQ bit was checked by dstockwell@chromium.org

[dstockwell, 2015-03-04 05:16:44]

The patchset sent to the CQ was uploaded after l-g-t-m from jl@opera.com
Link to the patchset: https://codereview.chromium.org/877523004/#ps40001 (title:
"Remove inheritance of [Measure]")

[commit-bot: I haz the power, 2015-03-04 05:17:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877523004/40001

[commit-bot: I haz the power, 2015-03-04 06:16:17]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191253