Bug 111417

Created attachment 191467 [details] Patch

Comment on attachment 191467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191467&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:73 > +my %specializedMainWorldBindings; > +# At the moment the hash map is empty. To fill it, add lines like this: > +# @{$specializedMainWorldBindings{"Class"}} = ({"attributeName" => 1}, {"methodName" => 1}); Why not have this be an IDL attribute? [V8SpecializeForMainWorld] ?

Comment on attachment 191467 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191467&action=review I'd recommend you split the patch into pieces. As a first step, how about just changing getters? When you're confident it works, you can enable it on setters and callbacks as well. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:73 >> +# @{$specializedMainWorldBindings{"Class"}} = ({"attributeName" => 1}, {"methodName" => 1}); > > Why not have this be an IDL attribute? [V8SpecializeForMainWorld] ? In my understanding, this hash map is just a temporary hack to proceed the work incrementally. In the near future, you'll be generating two object templates for all interfaces, right? If so, please add a comment like "FIXME: This is just a temporary hack to implement two ObjectTemplates for all interfaces. Once the work is completed, this should be removed." > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:596 > +template<class HolderContainer, class Wrappable> > +inline v8::Handle<v8::Value> toV8InMainWorld(${nativeType}* impl, const HolderContainer& container, Wrappable* wrappable) toV8InMainWorld => toV8FastForMainWorld > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:600 > + v8::Handle<v8::Object> wrapper = DOMDataStore::getWrapperInMainWorld(impl); Nit: getWrapperForMainWorld (Nit: Sorry, last time I recommended worldTypeInMainWorld, but it looks like xxxForMainWorld is a naming convention in V8 binding.) > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:909 > + my $isMainWorldSpecialization = shift; This could be $forMainWorldSuffix, so that you can pass "ForMainWorld" or "" (or omit the argument). Then call sites will become more readable. The same comment for other parts of the patch. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:918 > + my $functionSuffix = ""; > + if ($isMainWorldSpecialization) { > + $functionSuffix = "InMainWorld"; > + } Then you can remove this. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:951 > + my $toV8FastFunc = "toV8Fast"; > + my $functionSuffix = ""; > + if ($isMainWorldSpecialization) { > + $toV8FastFunc = "toV8InMainWorld"; > + $functionSuffix = "InMainWorld"; > + } Once you rename toV8InMainWorld to toV8FastForMainWorld, you can remove $toV8FastFunc. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2832 > + GenerateNormalAttrGetter($attribute, $interface, 0); This can be GenerateNormalAttrGetter($attribute, $interface). > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2838 > + GenerateNormalAttrGetter($attribute, $interface, 1); This can be GenerateNormalAttrGetter($attribute, $interface, "ForMainWorld"). > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3065 > + # FIXME: how to deal with overloaded functions? > + # Don't put any nonstandard functions into this table: This is non-trivial. Let's care about callbacks in a follow-up patch. You can enable the feature on only getters, as a starting point.

> > Why not have this be an IDL attribute? [V8SpecializeForMainWorld] ? > > In my understanding, this hash map is just a temporary hack to proceed the work incrementally. In the near future, you'll be generating two object templates for all interfaces, right? > > If so, please add a comment like "FIXME: This is just a temporary hack to implement two ObjectTemplates for all interfaces. Once the work is completed, this should be removed." Why not use an IDL attribute, even temporarily?

(In reply to comment #4) > > > Why not have this be an IDL attribute? [V8SpecializeForMainWorld] ? > > > > In my understanding, this hash map is just a temporary hack to proceed the work incrementally. In the near future, you'll be generating two object templates for all interfaces, right? > > Why not use an IDL attribute, even temporarily? Sounds reasonable.

I'm splitting this up, the first part is bug 111724.

Created attachment 193131 [details] Patch

The new patch generates bindings for getters and setters (not for callbacks).

(In reply to comment #8) > The new patch generates bindings for getters and setters (not for callbacks). but since you didn't modify any idl, it's not actually doing that yet, right?

It does. The code generator .pm just dummily generates custom versions for *everything* (except per-context attrs, didn't figure out how to do that).

(In reply to comment #10) > It does. The code generator .pm just dummily generates custom versions for *everything* (except per-context attrs, didn't figure out how to do that). In that case, we should see some speed up on the perf tests, right? Can you run the tests and post results?

Comment on attachment 193131 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193131&action=review Looks great. > Source/WebCore/ChangeLog:10 > + The new specialized bindings will be faster, because they don't need to > + do the "main world, isolated world or a worker" check, but can right > + away assume that we're in the main world. As jochen pointed out, you can measure the performance impact by using PerformanceTests/Bindings/first-child.html, which would be the most sensitive micro-benchmark to your change. Please paste the results here. (Actually I won't be much surprised if you cannot observe any performance improvement. When I tried the optimization last time, I couldn't observe any perf win.) > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2799 > + if (!$attrExt->{"V8EnabledPerContext"}) { Why do you want to exclude attributes with [V8EnabledPerContext]? (Either way you can fix it in a separate patch.) > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2809 > + if (!$attrExt->{"V8EnabledPerContext"}) { Ditto. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3062 > - if (!${enable_function}()) > + if (!${enable_function}()) Nit: Drop this change.

Created attachment 193274 [details] Patch

Some changes I made to the previous version: - Also generate toV8ForMainworld, not only toV8FastForMainWorld. - Added ASSERTS in toV8(Fast)*ForMainWorld, to assert that we indeed are in the main world. I ran the Bindings/* performance tests twice, and check which tests show a similar kind of improvement on both runs. Results: Better: /Bindings/document-implementation:Runs: ~ 12% better /Bindings/gc-tree:Time ~ 5% better /Bindings/insert-before:Runs ~ 3% better /Bindings/node-list-access:Runs ~ 3% better /Bindings/undefined-first-child:Runs ~ 3% better No measurable change: /Bindings/append-child:Runs /Bindings/create-element:Runs /Bindings/event-target-wrapper:Runs /Bindings/first-child:Runs /Bindings/gc-forest:Time /Bindings/gc-mini-tree:Time /Bindings/get-attribute:Runs /Bindings/get-element-by-id:Runs /Bindings/id-getter:Runs /Bindings/id-setter:Runs /Bindings/scroll-top:Runs /Bindings/set-attribute:Runs /Bindings/typed-array-construct-from-array:Runs /Bindings/typed-array-construct-from-same-type:Runs /Bindings/typed-array-construct-from-typed:Runs /Bindings/undefined-get-element-by-id:Runs /Bindings/undefined-id-getter:Runs Worse: /Bindings/get-elements-by-tag-name:Runs ~ 3% worse << flake?

(In reply to comment #14) > Better: > /Bindings/document-implementation:Runs: ~ 12% better > /Bindings/gc-tree:Time ~ 5% better > /Bindings/insert-before:Runs ~ 3% better > /Bindings/node-list-access:Runs ~ 3% better > /Bindings/undefined-first-child:Runs ~ 3% better Looks great, but I'm a bit suspicious of the accuracy. For example, undefined-first-child just returns v8::Undefined() when imp->firstChild() is 0. So I think that the performance won't be affected by your change.

Either way there is no reason that the patch regresses performance, I'm very happy to land it. We're just interested in performance improvement of the patch:)

Yep, the ~3 % changes are probably just noise. The ~ 12 % I can somewhat believe.

(In reply to comment #17) > Yep, the ~3 % changes are probably just noise. The ~ 12 % I can somewhat believe. Hmm, still I wonder why you got 12% speed-up in document-implementation.html whereas you didn't get speed-up in first-child.html. I think they are essentially the same in that they return a ScriptWrapperable to V8. (You don't need to investigate the issue though.)

Comment on attachment 193274 [details] Patch Clearing flags on attachment: 193274 Committed r145896: <http://trac.webkit.org/changeset/145896>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 112443

Created attachment 193690 [details] Patch

Attachment 193690 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/scripts/test/V8/V8Float64Array.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8Float64Array.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestCustomNamedGetter.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestCustomNamedGetter.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventConstructor.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventConstructor.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventTarget.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventTarget.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestException.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestException.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestInterface.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestMediaQueryListListener.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestMediaQueryListListener.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestNode.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestNode.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.h', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestTypedefs.h', u'Source/WebCore/bindings/v8/DOMDataStore.h', u'Source/WebCore/bindings/v8/V8DOMConfiguration.cpp', u'Source/WebCore/bindings/v8/V8DOMConfiguration.h', u'Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8EventTargetCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8IDBAnyCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8MicroDataItemValueCustom.cpp', u'Source/WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp']" exit_code: 1 Source/WebCore/bindings/v8/V8DOMConfiguration.h:83: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 193690 [details] Patch rs=me

I reverted this because it made the data segment of the executable (in this case, chrome, though the same thing is visible in DumpRenderTree) grow by ~ 140 K. In the new version, I don't create a new attribute array for storing the "forMainWorld" function pointers, but modify the existing attribute array to contain it. So now the data segment size increase only by ~ 40 K.

Created attachment 193694 [details] Patch

(In reply to comment #25) > I reverted this because it made the data segment of the executable (in this case, chrome, though the same thing is visible in DumpRenderTree) grow by ~ 140 K. > > In the new version, I don't create a new attribute array for storing the "forMainWorld" function pointers, but modify the existing attribute array to contain it. So now the data segment size increase only by ~ 40 K. Good idea! Can the 40 K increase roughly be explained by the attrGetter/attrSetter duplication? (I'd just like to confirm that we're not doing something ridiculous.)

Comment on attachment 193694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193694&action=review > Source/WebCore/bindings/v8/DOMDataStore.h:90 > + if (mainWorldWrapperIsStoredInObject(object) && isMainWorldObject(object)) { > + if (LIKELY(!DOMWrapperWorld::isolatedWorldsExist())) Why is this check needed? If I'm not missing something, the point of this patch is removing this check. (With this check, I don't understand why you observed performance improvement...) > Source/WebCore/bindings/v8/V8DOMConfiguration.h:61 > template<class ObjectOrTemplate> > static inline void configureAttribute(v8::Handle<ObjectOrTemplate> instance, v8::Handle<ObjectOrTemplate> prototype, const BatchedAttribute& attribute, v8::Isolate*) Nit: Shall we add a FIXME that says this method should be deprecated?

Comment on attachment 193694 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193694&action=review >> Source/WebCore/bindings/v8/DOMDataStore.h:90 >> + if (LIKELY(!DOMWrapperWorld::isolatedWorldsExist())) > > Why is this check needed? If I'm not missing something, the point of this patch is removing this check. > > (With this check, I don't understand why you observed performance improvement...) Ah yes. Thanks for pointing that out. Also, below, we call mainWorldStore() instead of resolving the current world (like in the version above). Probably this was still a perf boost because in the perf tests, isolated worlds didn't exist.

Kentaro: The data segment size increase was because the previous version added this extra array for the ForMainWorld function pointers. The new version doesn't. But, I don't think we want this version either; this is increasing the total executable size by 1 M. :/ So we'd probably want to select which functions we specialize with an IDL attribute.

Oh, and to answer your previous question: The per-context functions are left out of this patch, because plumbing the world type to be available in the installPerContextProperties functions is non-trivial. (I would need to add it to "wrap" and lots of other binding funcs.)

(In reply to comment #30) > Kentaro: > > The data segment size increase was because the previous version added this extra array for the ForMainWorld function pointers. The new version doesn't. > > But, I don't think we want this version either; this is increasing the total executable size by 1 M. :/ > > So we'd probably want to select which functions we specialize with an IDL attribute. Hmm, probably. Assuming that each getter/setter requires 200 B and there are 3000 getters/setters (I didn't count it, I just estimated by the fact that there are 600 IDL files), we cannot avoid consuming 200 B * 3000 = 600 KB, approximately 1 MB, which wouldn't be acceptable. In the selective duplication approach, are you going to add two templates for all interfaces but generate two getters/setters/methods only for getters/setters/methods that have [PerWorldAccess] ? Or do you have another idea in mind ?

(In reply to comment #31) > Oh, and to answer your previous question: > > The per-context functions are left out of this patch, because plumbing the world type to be available in the installPerContextProperties functions is non-trivial. (I would need to add it to "wrap" and lots of other binding funcs.) Makes sense to me. Thanks. If you take an approach where some interfaces have two templates and others have one template (which is technically possible), I'm afraid that the code base will become very messy and buggy. My question is what would be a good balancing point between clean code base and an increase in the executable file size.

After the patch in bug 111724 landed, we already have 2 different templates for main and non-main worlds. So I'd just selectively customize some of them and leave the others as they are now.

(In reply to comment #34) > After the patch in bug 111724 landed, we already have 2 different templates for main and non-main worlds. So I'd just selectively customize some of them and leave the others as they are now. OK, we (already) have two templates for all interfaces. So in this patch, we can just try to generate two getters/settters for DOM attributes that have [PerWorldDOMAccess] ?

Exactly.

Comment on attachment 193694 [details] Patch Attachment 193694 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17184376 New failing tests: storage/websql/sql-data-types.html

(In reply to comment #36) > Exactly. Sounds good to me. I hope it will be an easy work for you (I guess you will just need to add a check about [V8PerWorldDOMAccess]).

Created attachment 193857 [details] Patch

The latest patch adds a IDL attribute (V8PerWorldBindings) and specializes some getters / setters based on that. For starters, I specialized all the attributes of Node and Element.

Comment on attachment 193857 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193857&action=review ok can you post perf results with that patch? > Source/WebCore/ChangeLog:12 > + This patch generates main world bindings for getters and setters of Node can you mention the name of the IDL attribute you introduced?

This patch is really cool. Thanks for working on this!

Some perf tests results. Same setup than last time (run twice, locally, only include tests which show consistent results). Some of them are noise nevertheless. Better: create-element 15% gc-forest 10% gc-mini-tree 10% set-attribute 10% typed-array-construct-from-same-type 5% No change: append-child document-implementation event-target-wrapper first-child gc-tree get-attribute get-element-by-id get-elements-bytag-name id-getter id-setter insert-before scroll-top typed-array-construct-from-array typed-array-construct-from-typed typed-array-set-from-typed undefined-first-child undefined-get-element-by-id undefine-id-getter Worse: -

If you run the Dromeo/dom-* tests, I would expect you'd see a win for dom-traverse.html (and perhaps others).

Created attachment 193889 [details] Patch

Comment on attachment 193889 [details] Patch Rejecting attachment 193889 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-04', 'apply-attachment', '--no-update', '--non-interactive', 193889, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue Last 500 characters of output: e Source/WebCore/bindings/v8/custom/V8EventTargetCustom.cpp patching file Source/WebCore/bindings/v8/custom/V8IDBAnyCustom.cpp patching file Source/WebCore/bindings/v8/custom/V8MicroDataItemValueCustom.cpp patching file Source/WebCore/bindings/v8/custom/V8WorkerContextCustom.cpp patching file Source/WebCore/dom/Element.idl patching file Source/WebCore/dom/Node.idl Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://webkit-commit-queue.appspot.com/results/17247092

Created attachment 193911 [details] Patch

(Forgot to add that the expected executable size increase because of this change is around 100 K.)

Comment on attachment 193911 [details] Patch Clearing flags on attachment: 193911 Committed r146259: <http://trac.webkit.org/changeset/146259>

All reviewed patches have been landed. Closing bug.

Comment on attachment 193911 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=193911&action=review Cool! > Source/WebCore/bindings/v8/DOMDataStore.h:89 > + if (mainWorldWrapperIsStoredInObject(object) && isMainWorldObject(object)) This branch is resolved statically and removed by a compiler, right?

> This branch is resolved statically and removed by a compiler, right? yes, but it's wrong. the isMainWorldObject(object) part should be removed, since it limits this optimization to nodes. Also, isMainWorldObject is incorrectly named. It should be isMainThreadObject, since that's what it's trying to check