Bug 86175

(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?

(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.

Created attachment 141374 [details] WIP

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.

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.

(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.

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

(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.

Created attachment 141702 [details] Patch

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

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

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

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

Created attachment 141822 [details] Patch

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

Created attachment 141836 [details] Patch

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?

Created attachment 141865 [details] Patch

Created attachment 141867 [details] Patch

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.

Comment on attachment 141867 [details] Patch Great! The change looks much better now.

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

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

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

Created attachment 141900 [details] Patch for landing

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

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

Created attachment 142183 [details] Patch for landing

Comment on attachment 142183 [details] Patch for landing Clearing flags on attachment: 142183 Committed r117242: <http://trac.webkit.org/changeset/117242>

All reviewed patches have been landed. Closing bug.