Bug 86175 – HasCustomWillOrDidRecalcStyleFlag and family should live in a bit.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

Following flags are used to indicate that the node has some special callback functions.

- HasCustomWillOrDidRecalcStyleFlag
- HasCustomStyleForRendererFlag
- IsFrameOwnerElementFlag

We can replace them with a flag namely, say, HasCustomCallbacks.
And have optional virtual method to check which types of callbacks the node actually has.
Since majority of node doesn't have none of them and even single bit can work for early-out check.

[Ryosuke Niwa]

(In reply to comment #0)
> Following flags are used to indicate that the node has some special callback functions.
> 
> - HasCustomWillOrDidRecalcStyleFlag
> - HasCustomStyleForRendererFlag
> - IsFrameOwnerElementFlag
> 
> We can replace them with a flag namely, say, HasCustomCallbacks.
> And have optional virtual method to check which types of callbacks the node actually has.
> Since majority of node doesn't have none of them and even single bit can work for early-out check.

Why do we need those extra bits? Also adding virtual function calls don't seem like a good idea. Do we have any data on how rare these flags are?

[Hajime Morrita]

(In reply to comment #1)
> (In reply to comment #0)
> > Following flags are used to indicate that the node has some special callback functions.
> > 
> > - HasCustomWillOrDidRecalcStyleFlag
> > - HasCustomStyleForRendererFlag
> > - IsFrameOwnerElementFlag
> > 
> > We can replace them with a flag namely, say, HasCustomCallbacks.
> > And have optional virtual method to check which types of callbacks the node actually has.
> > Since majority of node doesn't have none of them and even single bit can work for early-out check.
> 
> Why do we need those extra bits? Also adding virtual function calls don't seem like a good idea. Do we have any data on how rare these flags are?
I need one for introduce InShadow and I'm expecting one more for another shadow purpose.
Also, this sharing allows us to more aggressive de-virtualization.
FYI elements with these flags are frame, iframe, plugin, form controls and svg.

In general people use flags if it's available. I made 3-4 bits for purpose I mentioned above.
But all bits were taken by other folks before I use it. I hope I could've reserve them.

[Hajime Morrita]

Created attachment 141374 [details]
WIP

[Hajime Morrita]

The assumption here is that the majority of nodes are trivial(non-replaced) elements or Text.

My whole plan under Bug 83268 is making them to run fast.
Ideally, if the element has no custom callbacks (and this is true for these trivial nodes),
they can be traversed with no virtual function call and hopefully with many inlining. 
Loop externalization and devirtualization is a pattern to achieve it.

[Ryosuke Niwa]

Comment on attachment 141374 [details]
WIP

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

> Source/WebCore/dom/NodeCustomCallbacks.h:88
> +    NodeCustomCallbacks* instance = node->getCustomCallbacks(&mask);
> +    return (mask & type) ? instance : 0;

It's kind of silly to have getCustomCallbacks return this pointer.
I'd suggest making getCustomCallbacks return a pointer to a member function instead.
That way, we'll be replacing one virtual function call to willRecalcStyle, didRecalcStyle, etc... by another one to getCustomCallbacks
instead of adding an extra virtual function call.

[Hajime Morrita]

(In reply to comment #5)
> (From update of attachment 141374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141374&action=review
> 
> > Source/WebCore/dom/NodeCustomCallbacks.h:88
> > +    NodeCustomCallbacks* instance = node->getCustomCallbacks(&mask);
> > +    return (mask & type) ? instance : 0;
> 
> It's kind of silly to have getCustomCallbacks return this pointer.
> I'd suggest making getCustomCallbacks return a pointer to a member function instead.
> That way, we'll be replacing one virtual function call to willRecalcStyle, didRecalcStyle, etc... by another one to getCustomCallbacks
> instead of adding an extra virtual function call.
The problem here is that each callback has different signature.

Also ,the cost desn't come from virtual functions specifically in many case,
but from polymorphic function call in general:
Compilers cannot inline them and optimization round the call site is prevented,
CPU has hard time to predict its destination. So replacing virtual function with function pointer won't help much here.

[Hajime Morrita]

But I agree that single v-call is better than double.
Looking into another approach now.

[Ryosuke Niwa]

(In reply to comment #6)
> The problem here is that each callback has different signature.
> 
> Also, the cost desn't come from virtual functions specifically in many case,
> but from polymorphic function call in general:
> Compilers cannot inline them and optimization round the call site is prevented,
> CPU has hard time to predict its destination. So replacing virtual function with function pointer won't help much here.

Really? I thought modern CPUs do a pretty decent job on indirect branch predictions. Also, passing a function pointer doesn't not require accessing the vtable, which may impact the the cache hit rate, etc... On the other hand, if you're already calling one virtual function, then the vtable, etc... are likely to be on cache anyway so it probably doesn't help as much.

[Hajime Morrita]

Created attachment 141702 [details]
Patch

[Hajime Morrita]

I realized that the last change was overkill. It can be simpler.

[Build Bot]

Comment on attachment 141702 [details]
Patch

Attachment 141702 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12702021

[WebKit Review Bot]

Comment on attachment 141702 [details]
Patch

Attachment 141702 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12679538

New failing tests:
editing/inserting/5607069-2.html
editing/pasteboard/nested-blocks-with-text-field.html
editing/pasteboard/copy-null-characters.html
fast/canvas/webgl/shader-precision-format.html
fast/forms/suggested-value-after-setvalue.html
fast/forms/form-attribute.html
fast/forms/suggested-value.html
http/tests/inspector/resource-har-pages.html
http/tests/security/sandboxed-iframe-origin-add.html
http/tests/security/xss-DENIED-iframe-src-alias.html
editing/pasteboard/drag-and-drop-inputimage-contenteditable.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNodeNS.html
editing/inserting/5607069-3.html
editing/selection/dump-as-markup.html
fast/dom/Attr/child-nodes-cache.html
editing/selection/dump-as-markup-form-text.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-getAttribute-value.html
fast/dom/insertedIntoDocument-sibling.html
http/tests/security/javascriptURL/javascriptURL-execution-context-frame-src-setAttributeNode.html
fast/dom/insertedIntoDocument-child.html
editing/pasteboard/drag-and-drop-objectimage-contenteditable.html

[WebKit Review Bot]

Created attachment 141728 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

Created attachment 141822 [details]
Patch

[Build Bot]

Comment on attachment 141822 [details]
Patch

Attachment 141822 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12701216

[Hajime Morrita]

Created attachment 141836 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 141836 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:2122
> +    return PassRefPtr<RenderStyle>();

We can't return 0 here?

> Source/WebCore/dom/Text.cpp:298
> +void Text::willRecalcTextStyle(StyleChange)
> +{
> +    ASSERT(hasCustomCallbacks());
> +}

Shouldn't we assert that this method is never called?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:250
> +PassRefPtr<RenderStyle> HTMLPlugInImageElement::customStyleForRenderer(bool& handled)
>  {
> -    ASSERT(m_customStyleForPageCache);
> +    if (!m_customStyleForPageCache) {
> +        ASSERT(!handled);
> +        return PassRefPtr<RenderStyle>();
> +    }
> +
> +    handled = true;

It appears that the only reason customStyleForRenderer takes a boolean is because of this function.
Can't we replace clearHasCustomStyleForRenderer and setHasCustomStyleForRenderer by clearHasCustomCallback and setHasCustomStyleForRenderer instead?

[Hajime Morrita]

Created attachment 141865 [details]
Patch

[Hajime Morrita]

Created attachment 141867 [details]
Patch

[Hajime Morrita]

Hi Ryosuke, thanks for the quick feedback!

(In reply to comment #17)
> (From update of attachment 141836 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141836&action=review
> 
> > Source/WebCore/dom/Element.cpp:2122
> > +    return PassRefPtr<RenderStyle>();
> 
> We can't return 0 here?
We can. Fixed.

> 
> > Source/WebCore/dom/Text.cpp:298
> > +void Text::willRecalcTextStyle(StyleChange)
> > +{
> > +    ASSERT(hasCustomCallbacks());
> > +}
> 
> Shouldn't we assert that this method is never called?
Right. Replaced with ASSERT_NOT_REACHED().

> 
> > Source/WebCore/html/HTMLPlugInImageElement.cpp:250
> > +PassRefPtr<RenderStyle> HTMLPlugInImageElement::customStyleForRenderer(bool& handled)
> >  {
> > -    ASSERT(m_customStyleForPageCache);
> > +    if (!m_customStyleForPageCache) {
> > +        ASSERT(!handled);
> > +        return PassRefPtr<RenderStyle>();
> > +    }
> > +
> > +    handled = true;
> 
> It appears that the only reason customStyleForRenderer takes a boolean is because of this function.
> Can't we replace clearHasCustomStyleForRenderer and setHasCustomStyleForRenderer by clearHasCustomCallback and setHasCustomStyleForRenderer instead?

Good point. Changed to create RenderStyle in normal way instead of returning zero.
Then we no longer need the out-parameter.

[Ryosuke Niwa]

Comment on attachment 141867 [details]
Patch

Great! The change looks much better now.

[Hajime Morrita]

(In reply to comment #21)
> (From update of attachment 141867 [details])
> Great! The change looks much better now.
Thanks for this another take!

[WebKit Review Bot]

Comment on attachment 141867 [details]
Patch

Attachment 141867 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12694543

New failing tests:
http/tests/appcache/deferred-events-delete-while-raising-timer.html
accessibility/aria-roles.html
accessibility/aria-disabled.html
accessibility/aria-checkbox-checked.html
http/tests/appcache/main-resource-hash.html
accessibility/aria-labelledby-on-input.html
http/tests/appcache/deferred-events-delete-while-raising.html
accessibility/aria-label.html
http/tests/appcache/detached-iframe.html
http/tests/appcache/foreign-fallback.html
accessibility/aria-readonly.html
accessibility/input-file-causes-crash.html
http/tests/appcache/cyrillic-uri.html
http/tests/appcache/local-content.html
accessibility/aria-labelledby-overrides-aria-label.html
http/tests/appcache/fail-on-update-2.html
http/tests/appcache/fallback.html
accessibility/crash-with-noelement-selectbox.html
accessibility/crash-determining-aria-role-when-label-present.html
http/tests/appcache/destroyed-frame.html

[WebKit Review Bot]

Created attachment 141888 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

Created attachment 141900 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 141900 [details]
Patch for landing

Rejecting attachment 141900 [details] from commit-queue.

New failing tests:
editing/input/paste-text-ending-with-interchange-newline.html
css3/selectors3/xml/css3-modsel-24.xml
editing/input/paste-linebreak-into-initially-hidden-textarea.html
editing/pasteboard/4806874.html
css3/selectors3/xhtml/css3-modsel-24.xml
editing/input/div-first-child-rule-input.html
editing/selection/3690719.html
css3/selectors3/xhtml/css3-modsel-68.xml
editing/pasteboard/5665299.html
editing/selection/4895428-3.html
css3/selectors3/html/css3-modsel-24.html
editing/selection/drag-select-1.html
editing/input/caret-at-the-edge-of-input.html
css3/selectors3/xml/css3-modsel-23.xml
editing/selection/3690703-2.html
editing/selection/4975120.html
css3/selectors3/xhtml/css3-modsel-69.xml
css3/selectors3/xml/css3-modsel-69.xml
editing/pasteboard/drop-text-without-selection.html
editing/selection/3690703.html
css3/selectors3/html/css3-modsel-23.html
css3/selectors3/xml/css3-modsel-68.xml
css3/selectors3/html/css3-modsel-69.html
editing/inserting/before-after-input-element.html
editing/pasteboard/input-field-1.html
http/tests/inspector/resource-har-pages.html
css3/selectors3/xhtml/css3-modsel-23.xml
css3/selectors3/html/css3-modsel-68.html
Full output: http://queues.webkit.org/results/12680848

[WebKit Review Bot]

Created attachment 141912 [details]
Archive of layout-test-results from ec2-cq-02

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Hajime Morrita]

Created attachment 142183 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142183 [details]
Patch for landing

Clearing flags on attachment: 142183

Committed r117242: <http://trac.webkit.org/changeset/117242>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.