JavaScript bindings changed to call scriptExecutionContext virtual function?
I noticed that many JavaScript binding implementations are now calling the virtual function scriptExecutionContext on DOM nodes. This should never be done! That's a virtual function, so it's not as fast as calling document(). The virtual Node::scriptExecutionContext() function should be made private so that we can catch at least some cases of this programming mistake at compile time. When this change was made, this presumably slowed down DOM bindings, since document() is an inline function that expands to a single memory read and scriptExecutionContext() is a virtual function call. When was this done? Can the person who did it fix it? There are various techniques that can be used to fix this. One is to only call scriptExecutionContext in generic code, and use functions named document() in less generic code that's working with a Node rather than an EventTarget. Another is to break scriptExecutionContext into virtual and non- virtual parts as we do with functions like Node::prefix and Node::virtualPrefix. -- Darin
On Sun, Aug 2, 2009 at 1:13 AM, Darin Adler<darin@apple.com> wrote:
I noticed that many JavaScript binding implementations are now calling the virtual function scriptExecutionContext on DOM nodes. This should never be done! That's a virtual function, so it's not as fast as calling document(). The virtual Node::scriptExecutionContext() function should be made private so that we can catch at least some cases of this programming mistake at compile time.
When this change was made, this presumably slowed down DOM bindings, since document() is an inline function that expands to a single memory read and scriptExecutionContext() is a virtual function call.
When was this done? Can the person who did it fix it?
Do you mean the two calls to scriptExecutionContext() in JSNodeCustom.cpp? Those appear to have been added in http://trac.webkit.org/changeset/40675 by Sam Weinig (reviewed by Darin Adler). If you mean one of the other 36 occurrences of scriptExecutionContext, I'm not sure, but we can certainly track it down. Is there a benchmark that shows the performance issue? That will be helpful in making sure we've fixed the problem.
There are various techniques that can be used to fix this.
One is to only call scriptExecutionContext in generic code, and use functions named document() in less generic code that's working with a Node rather than an EventTarget.
Another is to break scriptExecutionContext into virtual and non-virtual parts as we do with functions like Node::prefix and Node::virtualPrefix.
-- Darin
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Sun, Aug 2, 2009 at 1:41 AM, Adam Barth<abarth@webkit.org> wrote:
On Sun, Aug 2, 2009 at 1:13 AM, Darin Adler<darin@apple.com> wrote:
I noticed that many JavaScript binding implementations are now calling the virtual function scriptExecutionContext on DOM nodes. This should never be done! That's a virtual function, so it's not as fast as calling document(). The virtual Node::scriptExecutionContext() function should be made private so that we can catch at least some cases of this programming mistake at compile time.
When this change was made, this presumably slowed down DOM bindings, since document() is an inline function that expands to a single memory read and scriptExecutionContext() is a virtual function call.
When was this done? Can the person who did it fix it?
Do you mean the two calls to scriptExecutionContext() in JSNodeCustom.cpp? Those appear to have been added in http://trac.webkit.org/changeset/40675 by Sam Weinig (reviewed by Darin Adler).
Actually, that change just moved code that was actually introduced in http://trac.webkit.org/changeset/38063 by Alexey Proskuryakov (reviewed by Darin Adler).
If you mean one of the other 36 occurrences of scriptExecutionContext, I'm not sure, but we can certainly track it down.
Is there a benchmark that shows the performance issue? That will be helpful in making sure we've fixed the problem.
There are various techniques that can be used to fix this.
One is to only call scriptExecutionContext in generic code, and use functions named document() in less generic code that's working with a Node rather than an EventTarget.
Another is to break scriptExecutionContext into virtual and non-virtual parts as we do with functions like Node::prefix and Node::virtualPrefix.
-- Darin
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Sun, Aug 2, 2009 at 1:45 AM, Adam Barth<abarth@webkit.org> wrote:
On Sun, Aug 2, 2009 at 1:41 AM, Adam Barth<abarth@webkit.org> wrote:
On Sun, Aug 2, 2009 at 1:13 AM, Darin Adler<darin@apple.com> wrote:
I noticed that many JavaScript binding implementations are now calling the virtual function scriptExecutionContext on DOM nodes. This should never be done! That's a virtual function, so it's not as fast as calling document(). The virtual Node::scriptExecutionContext() function should be made private so that we can catch at least some cases of this programming mistake at compile time.
When this change was made, this presumably slowed down DOM bindings, since document() is an inline function that expands to a single memory read and scriptExecutionContext() is a virtual function call.
When was this done? Can the person who did it fix it?
Do you mean the two calls to scriptExecutionContext() in JSNodeCustom.cpp? Those appear to have been added in http://trac.webkit.org/changeset/40675 by Sam Weinig (reviewed by Darin Adler).
Actually, that change just moved code that was actually introduced in http://trac.webkit.org/changeset/38063 by Alexey Proskuryakov (reviewed by Darin Adler).
In any case: https://bugs.webkit.org/show_bug.cgi?id=27931 I'll have a patch shortly. Please let me know if there are other call sites you'd like changed.
If you mean one of the other 36 occurrences of scriptExecutionContext, I'm not sure, but we can certainly track it down.
Is there a benchmark that shows the performance issue? That will be helpful in making sure we've fixed the problem.
There are various techniques that can be used to fix this.
One is to only call scriptExecutionContext in generic code, and use functions named document() in less generic code that's working with a Node rather than an EventTarget.
Another is to break scriptExecutionContext into virtual and non-virtual parts as we do with functions like Node::prefix and Node::virtualPrefix.
-- Darin
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Aug 2, 2009, at 2:05 AM, Adam Barth wrote:
In any case:
https://bugs.webkit.org/show_bug.cgi?id=27931
I'll have a patch shortly. Please let me know if there are other call sites you'd like changed.
Thanks! Once you pointed out this was not due to a recent change, I had resolved to fix it myself, but I’m glad you’re going to tackle it. Generally speaking no call site that has a Node* should call scriptExecutionContext(). So rather than listing the call sites I want changed, I’d instead suggest that we make Node::scriptExecutionContext () private and then make sure no Node member functions or members functions of friend classes call the function. And the same is true of most other classes derived from EventTarget. -- Darin
On Sun, Aug 2, 2009 at 2:09 AM, Darin Adler<darin@apple.com> wrote:
On Aug 2, 2009, at 2:05 AM, Adam Barth wrote:
In any case:
https://bugs.webkit.org/show_bug.cgi?id=27931
I'll have a patch shortly. Please let me know if there are other call sites you'd like changed.
Thanks! Once you pointed out this was not due to a recent change, I had resolved to fix it myself, but I’m glad you’re going to tackle it.
Generally speaking no call site that has a Node* should call scriptExecutionContext(). So rather than listing the call sites I want changed, I’d instead suggest that we make Node::scriptExecutionContext() private and then make sure no Node member functions or members functions of friend classes call the function.
I tried this, and it looks like there are a bunch of instances in the generated bindings that weren't found by cscope. Basically, this line of CodeGeneartorJS needs to know whether impl is a subclass of Node*: push(@implContent, " JSDOMGlobalObject* globalObject = toJSDOMGlobalObject(imp->scriptExecutionContext());\n"); It's a bit too late at night for me to wrap my mind around the code generator, but that's a clear next step. Adam
In this case, we could probably reuse the isNodeSubType from CodeGeneratorV8.pm (http://trac.webkit.org/browser/trunk/WebCore/bindings/scripts/CodeGeneratorV...), which in turn uses FindParentsRecursively from CodeGenerator.pm (http://trac.webkit.org/browser/trunk/WebCore/bindings/scripts/CodeGenerator....). But I haven't looked at the problem in detail, just yelling random stuff from sidelines. :DG< On Sun, Aug 2, 2009 at 2:44 AM, Adam Barth<abarth@webkit.org> wrote:
On Sun, Aug 2, 2009 at 2:09 AM, Darin Adler<darin@apple.com> wrote:
On Aug 2, 2009, at 2:05 AM, Adam Barth wrote:
In any case:
https://bugs.webkit.org/show_bug.cgi?id=27931
I'll have a patch shortly. Please let me know if there are other call sites you'd like changed.
Thanks! Once you pointed out this was not due to a recent change, I had resolved to fix it myself, but I’m glad you’re going to tackle it.
Generally speaking no call site that has a Node* should call scriptExecutionContext(). So rather than listing the call sites I want changed, I’d instead suggest that we make Node::scriptExecutionContext() private and then make sure no Node member functions or members functions of friend classes call the function.
I tried this, and it looks like there are a bunch of instances in the generated bindings that weren't found by cscope. Basically, this line of CodeGeneartorJS needs to know whether impl is a subclass of Node*:
push(@implContent, " JSDOMGlobalObject* globalObject = toJSDOMGlobalObject(imp->scriptExecutionContext());\n");
It's a bit too late at night for me to wrap my mind around the code generator, but that's a clear next step.
Adam _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Aug 2, 2009, at 2:44 AM, Adam Barth wrote:
On Sun, Aug 2, 2009 at 2:09 AM, Darin Adler<darin@apple.com> wrote:
On Aug 2, 2009, at 2:05 AM, Adam Barth wrote:
In any case:
https://bugs.webkit.org/show_bug.cgi?id=27931
I'll have a patch shortly. Please let me know if there are other call sites you'd like changed.
Thanks! Once you pointed out this was not due to a recent change, I had resolved to fix it myself, but I’m glad you’re going to tackle it.
Generally speaking no call site that has a Node* should call scriptExecutionContext(). So rather than listing the call sites I want changed, I’d instead suggest that we make Node::scriptExecutionContext() private and then make sure no Node member functions or members functions of friend classes call the function.
I tried this, and it looks like there are a bunch of instances in the generated bindings that weren't found by cscope. Basically, this line of CodeGeneartorJS needs to know whether impl is a subclass of Node*:
push(@implContent, " JSDOMGlobalObject* globalObject = toJSDOMGlobalObject(imp->scriptExecutionContext());\n");
It's a bit too late at night for me to wrap my mind around the code generator, but that's a clear next step.
I think the code generator doesn't try to track the whole class hierarchy. Probably a simpler way to achieve the goal here would be with overloaded inline functions, so the C++ type system can do the work: inline ScriptExecutionContext* scriptExecutionContext(EventTarget* et) { return et->scriptExecutionContext(); } inline Document* scriptExecutionContext(Node* n) { return n->document(); } Alternately, EventTarget could have an inline nonvirtual member function that calls the virtual member function, and Node could override it with one that calls document(). Regards, Maciej
participants (4)
-
Adam Barth
-
Darin Adler
-
Dimitri Glazkov
-
Maciej Stachowiak