Bug 81198 – Improve isInsertionPoint performance.

• Component: DOM
• Status: RESOLVED INVALID

[Shinya Kawanaka]

From this discussion (http://code.google.com/p/chromium/issues/detail?id=117020), 
The refactoring for isInsertionPoint (http://trac.webkit.org/changeset/110151/) might regress the performance.

So let's try to make isInsertionPoint() faster.

[Shinya Kawanaka]

Created attachment 132002 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 132002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132002&action=review

How hot is this function? Maybe we should add a new node flag to avoid virtual function calls altogether?

> Source/WebCore/ChangeLog:9
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description.

[Shinya Kawanaka]

(In reply to comment #2)
> (From update of attachment 132002 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132002&action=review
> 
> How hot is this function? Maybe we should add a new node flag to avoid virtual function calls altogether?
> 
> > Source/WebCore/ChangeLog:9
> > +        Reviewed by NOBODY (OOPS!).
> 
> This line should appear before the description.

Before the regression, isContentElement (which is used instead of isInsertionPoint) is just a virtual function.

However,  isInsertionPoint is called for each element in NodeRenderingContext. I'm not sure we should add another flag for performance or just virtual function will do.

[Hajime Morrita]

Comment on attachment 132002 [details]
Patch

mikelawther is working on another suspicious patches. This can be wrong. Maybe it fixes the problem.
I hope we have some investigation using page cycler before having a blind change.

[Shinya Kawanaka]

(In reply to comment #4)
> (From update of attachment 132002 [details])
> mikelawther is working on another suspicious patches. This can be wrong. Maybe it fixes the problem.
> I hope we have some investigation using page cycler before having a blind change.

Yeah... I hope so.

[Sam Weinig]

Has there been any performance testing for this done yet?

[Ryosuke Niwa]

Comment on attachment 132002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132002&action=review

> Source/WebCore/dom/Node.h:219
> +    virtual bool isInsertionPoint() const { return false; }

Who returns true?

[Hajime Morrita]

Comment on attachment 132002 [details]
Patch

r- per Ryosuke and Sam's comment.
We need benchmark summary on ChangeLog for example.

[Shinya Kawanaka]

Let's make this INVALID, since we don't have any strong reason to make this faster.
Please feel free to reopen if you need.