Remove [PerWorldBindings] IDL attribute from non wrapper type attributes

Remove [PerWorldBindings] IDL attribute from non wrapper type attributes

Remove [PerWorldBindings] IDL attribute from non wrapper type attributes.
The 2 generated code paths are identical for non wrapper types (e.g.
DOMString) so there should be no performance benefit to having a separate
code path for the main world.

This reduces the release binary size by ~18Kb. I did not see any
regression on the Bindings performance tests locally.

This CL does not change attributes of non-wrapper type that use
[LogActivity=xxx] extended attribute because this would cause a behavior
change. This is sad though because those account for a much greater
binary size (~100 Kb in release).

This CL also adds an assertion to the bindings generator so that folks
stop doing this unintentionally.

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

[Inactive, 2014-04-29 14:56:03]

1. Let me know if I am missing something obvious as I know people have been
adding [PerWorldBindings] to DOMString attributes and I just don't see the
reason for it when looking at the generated code (the 2 code paths are
identical).
2. Does [LogActivity=*] still make sense on attributes where I removed
[PerWorldBindings]?

[haraken, 2014-04-29 15:49:22]

Thanks for doing this! We should really remove [PerWorldBindings]s that don't
affect performance. It's just bloating the binary size.

> This reduces the release binary size by ~115Kb.

Is this number on a Debug build or a Release build?


> 2. Does [LogActivity=*] still make sense on attributes where I removed
> [PerWorldBindings]?

I don't fully understand your question, but [LogActivity=*] still makes sense.
[LogActivity=*] controls whether we want to log the access to the DOM
attribute/method in specific worlds. If [LogAllWorlds] is specified, we want to
log the access in all worlds. If [LogAllWorlds] is not specified, we want to log
the access to only isolated worlds. On the other hand, [PerWorldBindings] just
controls whether we generate per-world bindings or not. In that sense,
[PerWorldBindings] and [LogXXX] are orthogonal concepts.

Did you confirm that the logging condition in the generated code is the same
after removing [PerWorldBindings] ? Although [PerWorldBindings] and [LogXXX] are
orthogonal concepts, I'm not sure if the IDL compiler is implemented correctly
when they are used separately.

[Inactive, 2014-04-29 15:52:34]

On 2014/04/29 15:49:22, haraken wrote:
> Thanks for doing this! We should really remove [PerWorldBindings]s that don't
> affect performance. It's just bloating the binary size.
> 
> > This reduces the release binary size by ~115Kb.
> 
> Is this number on a Debug build or a Release build?

This is for a Release build.

> 
> > 2. Does [LogActivity=*] still make sense on attributes where I removed
> > [PerWorldBindings]?
> 
> I don't fully understand your question, but [LogActivity=*] still makes sense.
> [LogActivity=*] controls whether we want to log the access to the DOM
> attribute/method in specific worlds. If [LogAllWorlds] is specified, we want
to
> log the access in all worlds. If [LogAllWorlds] is not specified, we want to
log
> the access to only isolated worlds. On the other hand, [PerWorldBindings] just
> controls whether we generate per-world bindings or not. In that sense,
> [PerWorldBindings] and [LogXXX] are orthogonal concepts.
> 
> Did you confirm that the logging condition in the generated code is the same
> after removing [PerWorldBindings] ? Although [PerWorldBindings] and [LogXXX]
are
> orthogonal concepts, I'm not sure if the IDL compiler is implemented correctly
> when they are used separately.

I will check the generated code for [LogActivity] but I am not familiar with
this attribute.

[Inactive, 2014-04-29 16:01:52]

Binary size before:
-rwxrwxr-x 1 chris chris 86780552 Apr 29 11:56 out/Release/content_shell

After:
-rwxrwxr-x 1 chris chris 86657352 Apr 29 11:59 out/Release/content_shell

So about 120Kb less.

[haraken, 2014-04-29 16:04:55]

On 2014/04/29 16:01:52, Chris Dumez wrote:
> Binary size before:
> -rwxrwxr-x 1 chris chris 86780552 Apr 29 11:56 out/Release/content_shell
> 
> After:
> -rwxrwxr-x 1 chris chris 86657352 Apr 29 11:59 out/Release/content_shell
> 
> So about 120Kb less.

This looks great!

[Inactive, 2014-04-29 16:05:50]

Here is the generated V8Element.cpp before:
http://pastebin.com/4UqShkRr

After:
http://pastebin.com/Vzrzh471

The diff:
http://pastebin.com/CNHtdrjn

The following attributes have [LogActivity] but I removed [PerWorldBindings]:
- innerHTML
- onwheel

I hope this helps.

[haraken, 2014-04-29 16:18:34]

On 2014/04/29 16:05:50, Chris Dumez wrote:
> Here is the generated V8Element.cpp before:
> http://pastebin.com/4UqShkRr
> 
> After:
> http://pastebin.com/Vzrzh471
> 
> The diff:
> http://pastebin.com/CNHtdrjn
> 
> The following attributes have [LogActivity] but I removed [PerWorldBindings]:
> - innerHTML
> - onwheel
> 
> I hope this helps.

hmm, this changes existing behavior and I'm not sure if it's OK.

- If you have [PerWorldBindings, LogActivity] on a DOM attribute/method and the
ActivityLogger is enabled in the main world, the DOM attribute/method is not
logged in the main world.

- If you have [LogActivity] on a DOM attribute/method and the ActivityLogger is
enabled in the main world, the DOM attribute/method is logged in the main world.

ulfar@ or pmarch@ or rdevlin@: Would you take a look at if the above behavior
change is OK?

[blink-reviews, 2014-04-29 16:32:39]

This is a large change in behavior with respect to the DOM activity logging
for extensions.

It's definitely not an OK change from our perspective, and it will cause a
significant performance regression in practice, for users that have
extensions with certain types of behavior.

Kentaro has plans to rework the activity logging so we shouldn't make this
change until those plans settle.  Kentaro: can we talk later today?

U


On Tue, Apr 29, 2014 at 9:18 AM, <haraken@chromium.org> wrote:

> On 2014/04/29 16:05:50, Chris Dumez wrote:
>
>> Here is the generated V8Element.cpp before:
>> http://pastebin.com/4UqShkRr
>>
>
>  After:
>> http://pastebin.com/Vzrzh471
>>
>
>  The diff:
>> http://pastebin.com/CNHtdrjn
>>
>
>  The following attributes have [LogActivity] but I removed
>> [PerWorldBindings]:
>> - innerHTML
>> - onwheel
>>
>
>  I hope this helps.
>>
>
> hmm, this changes existing behavior and I'm not sure if it's OK.
>
> - If you have [PerWorldBindings, LogActivity] on a DOM attribute/method
> and the
> ActivityLogger is enabled in the main world, the DOM attribute/method is
> not
> logged in the main world.
>
> - If you have [LogActivity] on a DOM attribute/method and the
> ActivityLogger is
> enabled in the main world, the DOM attribute/method is logged in the main
> world.
>
> ulfar@ or pmarch@ or rdevlin@: Would you take a look at if the above
> behavior
> change is OK?
>
>
> https://codereview.chromium.org/256263002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Inactive, 2014-04-29 16:56:38]

As I said, I am not familiar with [LogActivity] but I had a feeling it was
related to [PerWorldBindings].

Could we at least land a patch to get rid of [PerWorldBindings] for non-wrapper
type attributes that do *not* have [LogActivity] ? (I can add this exception to
the assert for now).

[blink-reviews, 2014-04-29 17:04:45]

What are those attributes that would be affected, then?

The PerWorldBindings were introduced to enable some optimizations, e.g., in
firstChild.  Would those optimizations be affected?  (Kentaro would know.)

U


On Tue, Apr 29, 2014 at 9:56 AM, <ch.dumez@samsung.com> wrote:

> As I said, I am not familiar with [LogActivity] but I had a feeling it was
> related to [PerWorldBindings].
>
> Could we at least land a patch to get rid of [PerWorldBindings] for
> non-wrapper
> type attributes that do *not* have [LogActivity] ? (I can add this
> exception to
> the assert for now).
>
>
> https://codereview.chromium.org/256263002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Inactive, 2014-04-29 17:07:22]

On 2014/04/29 17:04:45, blink-reviews_chromium.org wrote:
> What are those attributes that would be affected, then?
> 
> The PerWorldBindings were introduced to enable some optimizations, e.g., in
> firstChild.  Would those optimizations be affected?  (Kentaro would know.)

As I explain in the CL description, I am only removing [PerWorldBindings] for
attributes of non-wrapper type, since the two generated code paths (Main world
vs non-main world) are identical for such attributes. Node.firstChild returns a
Node, which is a wrapper type. Of course, [PerWorldBindings] is beneficial for
such attribute and is kept.

[haraken, 2014-04-30 00:47:14]

Thanks, I understand that we cannot simply remove [PerWorldBindings] from DOM
attributes/methods that have [LogActivity].

In summary:

(a) It is not OK to remove [PerWorldBindings] from DOM attributes/methods that
have [LogActivity].

(b) It is OK to remove [PerWorldBindings] from DOM attributes/methods that do
not have [LogActivity] that return non-wrapper type.

Chris: Would you update the CL so that this CL drops [PerWorldBindings] for (b)?
We can rework on (a) later.

[Nils Barth (inactive), 2014-04-30 00:58:12]

Hi Chris,
Thanks for doing this -- we've been wanting to reduce or remove
[PerWorldBindings]!

AFAICT:
* [PerWorldBindings] means we generate separate bindings for the main world and
isolated worlds
This is used for two things:
* Performance on certain performance-sensitive attributes, like firstChild
* Separate logging of use from main world and isolated world, if logged (via
[LogActivity]).

Per Chris's note, apparently the performance optimization is only for wrapper
types.
(haraken: could you confirm?)
However, the logging is relevant for all types, with is often DOMString.

Thus, sounds like:
* Ok to remove from non-wrapper, unlogged attributes, and to set an assert to
that effect.

haraken, WDYT?

Also, could you document this extended attribute, per conclusion of this
discussion?

It's one of the few remaining undocumented extended attributes:
http://www.chromium.org/blink/webidl/blink-idl-extended-attributes#TOC-Undocu...

[Nils Barth (inactive), 2014-04-30 00:59:01]

(Ok, sounds like we're agreed, per haraken's & mine notes.)

[Inactive, 2014-04-30 01:29:10]

I updated the CL accordingly. It is sad though because the [LogActivity]
attributes account for most of the binary size (~100Kb). So the win is now only
~18Kb. Hopefully we can address this later on.

[haraken, 2014-04-30 01:53:45]

On 2014/04/30 01:29:10, Chris Dumez wrote:
> I updated the CL accordingly. It is sad though because the [LogActivity]
> attributes account for most of the binary size (~100Kb). So the win is now
only
> ~18Kb. Hopefully we can address this later on.

LGTM.

We should really consider removing [PerWorldBindings] from [LogActivity]. If
you're interested in the work, I'm happy to explain the details.

[marja, 2014-04-30 11:11:08]

lgtm

https://codereview.chromium.org/256263002/diff/40001/Source/core/dom/Element.idl
File Source/core/dom/Element.idl (right):

https://codereview.chromium.org/256263002/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.idl:75: // [PerWorldBindings] attribute (Dictionary or
long) scrollLeft;
Remove from the comment too

[Inactive, 2014-04-30 12:19:03]

https://codereview.chromium.org/256263002/diff/40001/Source/core/dom/Element.idl
File Source/core/dom/Element.idl (right):

https://codereview.chromium.org/256263002/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.idl:75: // [PerWorldBindings] attribute (Dictionary or
long) scrollLeft;
On 2014/04/30 11:11:09, marja wrote:
> Remove from the comment too

Done.

[Inactive, 2014-04-30 12:19:31]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-04-30 12:19:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/256263002/60001

[commit-bot: I haz the power, 2014-04-30 12:19:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 12:19:44]

Failed to apply patch for Source/bindings/tests/idls/TestInterface.idl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/tests/idls/TestInterface.idl
  Hunk #1 FAILED at 52.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/bindings/tests/idls/TestInterface.idl.rej

Patch:       Source/bindings/tests/idls/TestInterface.idl
Index: Source/bindings/tests/idls/TestInterface.idl
diff --git a/Source/bindings/tests/idls/TestInterface.idl
b/Source/bindings/tests/idls/TestInterface.idl
index
17716c80bb44a0e8d732750c33bbbbb669bc7845..6c5ab97aa0adb95e61fb71270828d8424846b4e1
100644
--- a/Source/bindings/tests/idls/TestInterface.idl
+++ b/Source/bindings/tests/idls/TestInterface.idl
@@ -52,7 +52,6 @@
     static attribute DOMString staticStringAttribute;
 
     void voidMethodTestInterfaceEmptyArg(TestInterfaceEmpty
testInterfaceEmptyArg);
-    [PerWorldBindings] attribute DOMString perWorldBindingsStringAttribute;
     [PerWorldBindings] void voidMethod();
 
     // Anonymous indexed property operations

[Inactive, 2014-04-30 13:04:24]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-04-30 13:04:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/256263002/80001

[commit-bot: I haz the power, 2014-04-30 15:04:34]

Change committed as 172986