[V8] toFastV8 for non-Nodes
Created attachment 174973 [details] Patch
Comment on attachment 174973 [details] Patch patch is hitting an assertion
can you post a backtrace to the assertion?
Comment on attachment 174973 [details] Patch Attachment 174973 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14928005 New failing tests: inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html
Comment on attachment 174973 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174973&action=review The patch looks great. Marking r- due to the code duplication for AccessorInfo and Arguments. > Source/WebCore/ChangeLog:3 > + [V8] toFastV8 for non-Nodes I'm a bit confused with the title. Is this patch intending to use toV8Fast() for a case where IsDOMNodeType($type) is true? Nit: toV8Fast > Source/WebCore/ChangeLog:8 > + This patch makes most generated bindings use toFastV8 Ditto. > Source/WebCore/ChangeLog:10 > + Additionally, a toFastV8 for v8::Arguments was added Ditto. > Source/WebCore/ChangeLog:11 > + to make this most of thia optimization. Typo: thia. Sorry for nit-pickings. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:536 > +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const v8::Arguments& info, Node*) Nit: We're likely to use 'args' instead of 'info'. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:539 > + > +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const v8::Arguments& info, Node*) > +{ > + return toV8(impl, info.Holder(), info.GetIsolate()); > +} We might not want to duplicate this function for AccessorInfo and Arguments. Wouldn't it be possible to change the signature to v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, v8::Handle<v8::Object> holder, Node*); and thus eliminate the duplication? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:588 > + foreach my $argType ("AccessorInfo","Arguments") { Nit: "AccessorInfo", "Arguments" (One space after ,) > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:620 > +inline v8::Handle<v8::Value> toV8Fast(PassRefPtr< ${nativeType} > impl, const v8::Arguments& info, Node* holder) Nit: We are likely to use 'args' instead of 'info'. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4144 > + if ($getFastArgs) { Don't you need to check IsDOMNodeType($type) ?
Created attachment 177259 [details] Patch
Comment on attachment 174973 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174973&action=review >> Source/WebCore/ChangeLog:3 >> + [V8] toFastV8 for non-Nodes > > I'm a bit confused with the title. Is this patch intending to use toV8Fast() for a case where IsDOMNodeType($type) is true? > > Nit: toV8Fast fixed it means that the callback is from for a node subclass, but the object requested could be anything with a wrapper >> Source/WebCore/ChangeLog:8 >> + This patch makes most generated bindings use toFastV8 > > Ditto. ok >> Source/WebCore/ChangeLog:10 >> + Additionally, a toFastV8 for v8::Arguments was added > > Ditto. ok >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:539 >> +} > > We might not want to duplicate this function for AccessorInfo and Arguments. Wouldn't it be possible to change the signature to > > v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, v8::Handle<v8::Object> holder, Node*); > > and thus eliminate the duplication? ok >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:588 >> + foreach my $argType ("AccessorInfo","Arguments") { > > Nit: "AccessorInfo", "Arguments" (One space after ,) ok >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:620 >> +inline v8::Handle<v8::Value> toV8Fast(PassRefPtr< ${nativeType} > impl, const v8::Arguments& info, Node* holder) > > Nit: We are likely to use 'args' instead of 'info'. ok >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:4144 >> + if ($getFastArgs) { > > Don't you need to check IsDOMNodeType($type) ? not here, the presence of the argument implies there is a node to make the call from
Comment on attachment 177259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177259&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:589 > -inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const v8::AccessorInfo& info, Node* holder) > +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const v8::Local<v8::Object> holder, v8::Isolate* isolate, Node* node) This seems to imply that all callers need to call GetIsolate before calling toV8Fast. In the common path through this function, we previously avoided the GetIsolate call. Does calling that function eagerly affect performance (I would expect it to). It might be worth having two overloaded copies of this function: one for AccessorInfo and one for Arguments. We could even make it a template since the code would be identical. This function is always inlined, so there wouldn't be any cost to having two copies.
Comment on attachment 177259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177259&action=review > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:585 > + my $fastWrapper = $codeGenerator->IsSubType($interface, "Node") ? "impl->wrapper()" : "DOMDataStore::mainWorldStore()->get(impl)"; Very clever! > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1076 > + if (IsDOMNodeType($interfaceName)) { In principle, we should be able to use toV8Fast whenever $interfaceName is ScriptWrappable. We can have the compiler do the dispatch by exposing DOMDataStore::wrapperIsStoredInObject publicly. We can do that in a subsequent patch if you like.
Can you run dom-traverse to make sure we're not regressing? I suspect calling GetIsolate one extra time will show up on that perf test. (It should be fixable as described above.)
Comment on attachment 177259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177259&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:589 >> +inline v8::Handle<v8::Value> toV8Fast(${nativeType}* impl, const v8::Local<v8::Object> holder, v8::Isolate* isolate, Node* node) > > This seems to imply that all callers need to call GetIsolate before calling toV8Fast. In the common path through this function, we previously avoided the GetIsolate call. Does calling that function eagerly affect performance (I would expect it to). > > It might be worth having two overloaded copies of this function: one for AccessorInfo and one for Arguments. We could even make it a template since the code would be identical. This function is always inlined, so there wouldn't be any cost to having two copies. I had the two copies before but haraken didn't want it. I thought the Isolate was needed in all paths, but I see now that it's not in the fast path. I'll switch to a templatized method.
Created attachment 177476 [details] Patch
(In reply to comment #12) > Created an attachment (id=177476) [details] > Patch This time round I moved all the complexity to DOMDataStore inline templates. Seems much cleaner.
Note, this patch doesn't have the adjustments to the binding test classes. It's harder to read the patch, so I'll add that after an r+.
(In reply to comment #10) > Can you run dom-traverse to make sure we're not regressing? I suspect calling GetIsolate one extra time will show up on that perf test. (It should be fixable as described above.) Your comment no longer applies, but here are the numbers, run twice each with interwoven runs. Looks like a small perf bump. with patch: avg 3069.1348651348653 runs/s median 0 runs/s stdev 21.196854488648402 runs/s min 3009.99000999001 runs/s max 3088.81018981019 runs/s avg 3192.8747252747253 runs/s median 0 runs/s stdev 12.296375015512377 runs/s min 3163.9750249750246 runs/s max 3213.18981018981 runs/s without patch: avg 2963.217982017982 runs/s median 0 runs/s stdev 16.350015231822884 runs/s min 2920.834165834166 runs/s max 2987.306693306693 runs/s avg 2925.672727272727 runs/s median 0 runs/s stdev 6.087018068830624 runs/s min 2908.5304695304694 runs/s max 2936.0009990009994 runs/s
Comment on attachment 177476 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177476&action=review Looks great. > Source/WebCore/bindings/v8/DOMDataStore.h:65 > + static v8::Handle<v8::Object> getWrapperFast(T* object, const HolderContainer& container, Wrappable* wrappable) wrappable -> holder
Created attachment 177692 [details] Patch
Comment on attachment 177692 [details] Patch Clearing flags on attachment: 177692 Committed r136652: <http://trac.webkit.org/changeset/136652>
All reviewed patches have been landed. Closing bug.