[webkit-dev] JavaScript bindings changed to call scriptExecutionContext virtual function?

[Adam Barth]

On Sun, Aug 2, 2009 at 1:45 AM, Adam Barth<abarth at webkit.org> wrote:
> On Sun, Aug 2, 2009 at 1:41 AM, Adam Barth<abarth at webkit.org> wrote:
>> On Sun, Aug 2, 2009 at 1:13 AM, Darin Adler<darin at 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 at lists.webkit.org
>>> http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
>>>
>>
>