Bug 101116

Created attachment 172539 [details] Patch

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

Comment on attachment 172539 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172539&action=review > Source/WebCore/dom/Text.cpp:267 > + Node* parentForStyle = parent->isShadowRoot() ? > + toShadowRoot(parent)->shadowHost() : > + parent; This should probably be just one line.

Comment on attachment 172539 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172539&action=review When resetStyleInheritance is true we have no m_parentStyle in StyleResolver and then go down this code path: http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/css/StyleResolver.cpp&exact_package=chromium&q=StyleResolver.cpp&type=cs&l=1545 You need to have this same behavior for text nodes. > Source/WebCore/dom/Text.cpp:262 > // is a shadow root. Remove this comment since it doesn't apply now. > Source/WebCore/dom/Text.cpp:268 > + renderer->setStyle(parentForStyle->renderStyle()); This is wrong if you have resetStyleInheritance set because you unconditionally inherit from the shadow host. You need to expose the default style from inside the StyleResolver, perhaps with some abstraction like document()->styleResolver()->styleForText(this) that models the existing styleForElement.

Comment on attachment 172539 [details] Patch Thanks for looking this over, Elliot.

Thank you for reviewing. I missed reset-style-inheritance. I agree that I have to think about this. So.. however I would like to confirm what style we should apply to resetted text nodes. The reason why I'm thinking about this is: (1) we cannot directly apply any styles to text nodes. (2) if we have to check reset-style-inheritance flag, we have to modify NodeRendererFactory::createRendererIfNeeded() too, i.e. ---- before ----- Element* element = node->isElementNode() ? toElement(node) : 0; if (element) m_context.setStyle(element->styleForRenderer()); else if (RenderObject* parentRenderer = m_context.parentRenderer()) m_context.setStyle(parentRenderer->style()); ---- after ---- Element* element = node->isElementNode() ? toElement(node) : 0; if (element) m_context.setStyle(element->styleForRenderer()); else if (node->isTextNode()) m_context.setStyle(document()->styleResolver()->styleForText(node); // added else if (RenderObject* parentRenderer = m_context.parentRenderer()) m_context.setStyle(parentRenderer->style()); Since for each text node we have to use NodeRenderingContext to check reset-style-inheritance, I'm afraid of performance regression. Is this allowable regression? Best regards, Takashi Sakamoto

(In reply to comment #6) > Thank you for reviewing. > > I missed reset-style-inheritance. I agree that I have to think about this. > > So.. however I would like to confirm what style we should apply to resetted text nodes. The reason why I'm thinking about this is: > (1) we cannot directly apply any styles to text nodes. > (2) if we have to check reset-style-inheritance flag, we have to modify NodeRendererFactory::createRendererIfNeeded() too, i.e. > > ---- before ----- > Element* element = node->isElementNode() ? toElement(node) : 0; > if (element) > m_context.setStyle(element->styleForRenderer()); > else if (RenderObject* parentRenderer = m_context.parentRenderer()) > m_context.setStyle(parentRenderer->style()); > ---- after ---- > Element* element = node->isElementNode() ? toElement(node) : 0; > if (element) > m_context.setStyle(element->styleForRenderer()); > else if (node->isTextNode()) > m_context.setStyle(document()->styleResolver()->styleForText(node); // added > else if (RenderObject* parentRenderer = m_context.parentRenderer()) > m_context.setStyle(parentRenderer->style()); > > Since for each text node we have to use NodeRenderingContext to check reset-style-inheritance, I'm afraid of performance regression. Is this allowable regression? First of all, let's measure it. Our PerformanceTests has some parser-related tests which exercise not only the parser but also the style computation. It would be a good starting point.

(In reply to comment #6) > ... > Since for each text node we have to use NodeRenderingContext to check reset-style-inheritance, I'm afraid of performance regression. Is this allowable regression? isShadowRoot and isTextNode are bitfield checks and we only need this special case for text nodes that are direct descendants of shadow roots. Element* element = node->isElementNode() ? toElement(node) : 0; if (element) m_context.setStyle(element->styleForRenderer()); else if (node->isTextNode() && node->parentOrHostNode()->isShadowRoot()) m_context.setStyle(document()->styleResolver()->styleForText(node); else m_context.setStyle(m_context.parentRenderer()->style()); The last check is simplified as in Bug 102765 and since the conditional is on a bitfield it seems very unlikely there's a perf regression.

Created attachment 175145 [details] Patch

(In reply to comment #8) > (In reply to comment #6) > > ... > > Since for each text node we have to use NodeRenderingContext to check reset-style-inheritance, I'm afraid of performance regression. Is this allowable regression? > > isShadowRoot and isTextNode are bitfield checks and we only need this special case for text nodes that are direct descendants of shadow roots. > > Element* element = node->isElementNode() ? toElement(node) : 0; > if (element) > m_context.setStyle(element->styleForRenderer()); > else if (node->isTextNode() && node->parentOrHostNode()->isShadowRoot()) > m_context.setStyle(document()->styleResolver()->styleForText(node); > else > m_context.setStyle(m_context.parentRenderer()->style()); > > The last check is simplified as in Bug 102765 and since the conditional is on a bitfield it seems very unlikely there's a perf regression. Thank you. I will recreate a new patch and upload. Best regards, Takashi Sakamoto

Created attachment 175150 [details] Patch

Created attachment 175151 [details] Patch

Created attachment 175152 [details] Patch

Comment on attachment 175145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175145&action=review We can make createRendererIfNeeded faster by not creating a whole new NodeRenderingContext and skipping the parent traversal using an overload and passing in the context from NodeRenderFactory: StyleResolver::styleForText(Node* textNode, NodeRenderingContext& context) { ... } StyleResolver::styleForText(Node* textNode) { NodeRenderingContext context(textNode); return styleForText(textNode, context); } I'm not sure this optimization is needed, but what you have right now does traverse the parents repeatedly. > Source/WebCore/ChangeLog:8 > + Use NodeRenderingContext to solve styles of text nodes. resolve > Source/WebCore/ChangeLog:9 > + If text nodes are direct chilren of shadow roots, the text nodes children > Source/WebCore/css/StyleResolver.cpp:1795 > + NodeRenderingContext context(textNode); It seems kind of silly to create a whole new rendering context and do the ComposedShadowTreeWalker::findParent traversal during createRendererIfNeeded() when we already have all that information in createRendererIfNeeded. It might not actually matter for performance though.

Created attachment 175156 [details] Patch

Comment on attachment 175145 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175145&action=review Thank you for quickly reviewing. >> Source/WebCore/ChangeLog:8 >> + Use NodeRenderingContext to solve styles of text nodes. > > resolve Done. Thanks. >> Source/WebCore/ChangeLog:9 >> + If text nodes are direct chilren of shadow roots, the text nodes > > children Done. >> Source/WebCore/css/StyleResolver.cpp:1795 >> + NodeRenderingContext context(textNode); > > It seems kind of silly to create a whole new rendering context and do the ComposedShadowTreeWalker::findParent traversal during createRendererIfNeeded() when we already have all that information in createRendererIfNeeded. It might not actually matter for performance though. I see. I added styleForElement(Node*, NodeRenderingContext&).

Comment on attachment 175156 [details] Patch Looks good to me.

Created attachment 175183 [details] Patch

I modified the condition in createRendererIfNeeded, because I think, m_context.insertionPoint() is a little better than isShadowHost(node->parentOrHostNode()). Best regards, Takashi Sakamoto

Created attachment 175189 [details] Patch

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

Created attachment 175347 [details] Patch

What about performance?

Comment on attachment 175347 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175347&action=review This looks good to me, but I'm not a reviewer so I can't r+. Perhaps someone else can do that. :) > Source/WebCore/dom/NodeRenderingContext.cpp:252 > + m_context.setStyle(node->document()->styleResolver()->styleForText(m_context)); You can just move the Document* document = node->document(); line back to the top of the function again if you want too.

(In reply to comment #23) > What about performance? To test performance of this use the html5-full-render.html perf test. It's a good test of createRendererIfNeeded() and recalcTextStyle().

(In reply to comment #25) > (In reply to comment #23) > > What about performance? > > To test performance of this use the html5-full-render.html perf test. It's a good test of createRendererIfNeeded() and recalcTextStyle(). Thank you. I looked at PerfTest/Parser/*. So I found that this patch makes Parser/tiny-innerHTML much slower (10%-20% slower). The test updates innerHTML again and again: PerfTestRunner.measureRunsPerSecond({run:function() { var testDiv = document.createElement("div"); testDiv.style.display = "none"; document.body.appendChild(testDiv); for (var x = 0; x < 100000; x++) { testDiv.innerHTML = "This is a tiny HTML document"; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ } document.body.removeChild(testDiv); }}); So the newly added "if" , i.e. + else if (node->isTextNode() && (node->parentOrHostNode()->isShadowRoot() || m_context.insertionPoint())) + m_context.setStyle(node->document()->styleResolver()->styleForText(m_context)); is checked every time testDiv.innerHTML is updated. I couldn't find the way to avoid this performance regression. Best regards, Takashi Sakamoto

(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #23) > > > What about performance? > > > > To test performance of this use the html5-full-render.html perf test. It's a good test of createRendererIfNeeded() and recalcTextStyle(). > > Thank you. I looked at PerfTest/Parser/*. > > So I found that this patch makes Parser/tiny-innerHTML much slower (10%-20% slower). > The test updates innerHTML again and again: > Can you try this test but with ENABLE(DIALOG_ELEMENT) turned off and ENABLE(FULLSCREEN_API) turned off? It seems very suspicious that adding this check is such a big perf regression but those two feature flags don't cause similarly massive perf regressions.

> > Can you try this test but with ENABLE(DIALOG_ELEMENT) turned off and ENABLE(FULLSCREEN_API) turned off? It seems very suspicious that adding this check is such a big perf regression but those two feature flags don't cause similarly massive perf regressions. Another thing we can try is to use pprof and see the difference between before and after the change.

Created attachment 176199 [details] Patch

Thank you for advising, Elliott and Morita-san. Today I rebased my patch. So I found no big performance regression... I measured by using the command, "Tools/Scripts/run-perf-tests --release --no-build Parser/\*" and I used the Patch(2012-11-27 01:41 PST). So I'm now trying to use pperf. Best regards, Takashi Sakamoto

Created attachment 176674 [details] Patch

I created a spreadsheet for Parser/tiny-innerHTML, Parser/html5-full-render, Parser/html-parser: https://docs.google.com/spreadsheet/ccc?key=0AtdkWCxGaEc2dDhUcTlFanprMUR0cGU0QlB1eWNoMWc I guess, someone's refactoring would probably hide my change. Best regards, Takashi Sakamoto

(In reply to comment #32) > I guess, someone's refactoring would probably hide my change. Not sure what this means :) Can you elaborate?

(In reply to comment #33) > (In reply to comment #32) > > I guess, someone's refactoring would probably hide my change. > > Not sure what this means :) Can you elaborate? I found that some one changed createRendererIfNeeded in NodeRenderingContext.cpp. My first patch was for updating NodeRendererFactory::createRendererIfNeeded. But now NodeRenderingContext::createRendererForTextIfNeeded was implemented for just creating a renderer for a text node. I recreated my patch to update the createRendererForTextIfNeeded. I think, this change affected number of "if" (or ...) when testDiv.innerHTML = "This is a tiny HTML document" is executed. Best regards, Takashi Sakamoto

Comment on attachment 176674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176674&action=review > Source/WebCore/dom/Text.cpp:292 > + renderer->setStyle(parent->renderer()->style()); Having these tricky condition in two places is unfortunate and error prone. Can we put this inside StyleResolver::styleForText() somehow?

Created attachment 178150 [details] Patch

Comment on attachment 176674 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176674&action=review >> Source/WebCore/dom/Text.cpp:292 >> + renderer->setStyle(parent->renderer()->style()); > > Having these tricky condition in two places is unfortunate and error prone. > Can we put this inside StyleResolver::styleForText() somehow? I see. Done.

Comment on attachment 178150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178150&action=review > Source/WebCore/dom/NodeRenderingContext.cpp:285 > + if (m_node->parentOrHostNode()->isShadowRoot() || insertionPoint()) Apparently this check also can be part of styleForText().

Created attachment 178166 [details] Patch

Comment on attachment 178166 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178166&action=review > Source/WebCore/css/StyleResolver.cpp:1809 > + m_style->font().update(0); This empty style creation code looks like being copied from styleForElement(). can we share this between two method? > Source/WebCore/css/StyleResolver.cpp:1821 > + return parentNode->renderStyle(); This code is so cryptic and hard to see its correctness. Is it possible to just let stleForText(NodeRenderingContext) do this? Is it so slow?

Created attachment 178172 [details] Patch

Created attachment 178176 [details] Patch

Created attachment 178177 [details] Patch

Comment on attachment 178166 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178166&action=review >> Source/WebCore/css/StyleResolver.cpp:1809 >> + m_style->font().update(0); > > This empty style creation code looks like being copied from styleForElement(). can we share this between two method? I see. Done. >> Source/WebCore/css/StyleResolver.cpp:1821 >> + return parentNode->renderStyle(); > > This code is so cryptic and hard to see its correctness. > Is it possible to just let stleForText(NodeRenderingContext) do this? > Is it so slow? I agree that this code looks not-good. I heard that NodeRenderingContext is a little slow. So this code is to creating a fast path, i.e. avoiding creating NodeRenderingContext. If always using NodeRenderingContext here is ok, I would like to remove this.

Created attachment 178180 [details] Patch

Comment on attachment 178166 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178166&action=review >>> Source/WebCore/css/StyleResolver.cpp:1821 >>> + return parentNode->renderStyle(); >> >> This code is so cryptic and hard to see its correctness. >> Is it possible to just let stleForText(NodeRenderingContext) do this? >> Is it so slow? > > I agree that this code looks not-good. > I heard that NodeRenderingContext is a little slow. So this code is to creating a fast path, i.e. avoiding creating NodeRenderingContext. > If always using NodeRenderingContext here is ok, I would like to remove this. Done. I removed the code.

Comment on attachment 178180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178180&action=review Looks awesome :) > Source/WebCore/css/StyleResolver.cpp:1781 > +PassRefPtr<RenderStyle> StyleResolver::defaultStyleForElement() defaultStyleForNode() ? Text are not Elements. > Source/WebCore/css/StyleResolver.cpp:1794 > +PassRefPtr<RenderStyle> StyleResolver::styleForText(Node* textNode) This should just take Text* and you can remove the isTextNode() assert below. > Source/WebCore/dom/NodeRenderingContext.cpp:285 > + if (UNLIKELY(resetStyleInheritance())) Does the UNLIKELY actually make a difference? ojan has said it doesn't usually matter (and produces identical assembly).

Comment on attachment 178180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178180&action=review >> Source/WebCore/css/StyleResolver.cpp:1781 >> +PassRefPtr<RenderStyle> StyleResolver::defaultStyleForElement() > > defaultStyleForNode() ? Text are not Elements. In this context, he's just initializing defaults for element style resolution. >> Source/WebCore/css/StyleResolver.cpp:1794 >> +PassRefPtr<RenderStyle> StyleResolver::styleForText(Node* textNode) > > This should just take Text* and you can remove the isTextNode() assert below. Good point! >> Source/WebCore/dom/NodeRenderingContext.cpp:285 >> + if (UNLIKELY(resetStyleInheritance())) > > Does the UNLIKELY actually make a difference? ojan has said it doesn't usually matter (and produces identical assembly). It's called UNLIKELY, because it is unlikely to have any effect on your code.

Created attachment 178955 [details] Patch

Comment on attachment 178180 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178180&action=review >>> Source/WebCore/css/StyleResolver.cpp:1781 >>> +PassRefPtr<RenderStyle> StyleResolver::defaultStyleForElement() >> >> defaultStyleForNode() ? Text are not Elements. > > In this context, he's just initializing defaults for element style resolution. Yes. Thank you Dimitri. I would like to say so. >>> Source/WebCore/css/StyleResolver.cpp:1794 >>> +PassRefPtr<RenderStyle> StyleResolver::styleForText(Node* textNode) >> >> This should just take Text* and you can remove the isTextNode() assert below. > > Good point! Thanks. Done. >>> Source/WebCore/dom/NodeRenderingContext.cpp:285 >>> + if (UNLIKELY(resetStyleInheritance())) >> >> Does the UNLIKELY actually make a difference? ojan has said it doesn't usually matter (and produces identical assembly). > > It's called UNLIKELY, because it is unlikely to have any effect on your code. I removed UNLIKELY. Done.

Comment on attachment 178955 [details] Patch Looks good now. Thanks for the fix!

Comment on attachment 178955 [details] Patch Clearing flags on attachment: 178955 Committed r137418: <http://trac.webkit.org/changeset/137418>

All reviewed patches have been landed. Closing bug.