Make renderer construction less generic

• Component: DOM
• Status: RESOLVED FIXED

[Antti Koivisto]

The renderer construction code currently operates on Nodes and is very generic. In reality only Element and Text nodes can have renderers and the Text case is much simpler.

[Antti Koivisto]

Created attachment 175855 [details]
patch

[Eric Seidel (no email)]

Curious if you see a perf effect from this change.  Several of the microbenchmarks in PerformanceTests, including Parser/html5-full-render.html hit render creation pretty hard.

[Eric Seidel (no email)]

I guess in my initial reading, I'm not really for or against this change.  It doesn't seem much simpler to me.  But I like the idea of someone trying to make all of this ancient code simpler/faster.  I'm just not sure what your end goal here is. :)

I agree that there are only two cases of interest (text and element nodes), and that should always be the case.  So perhaps special casing them (as you've done here) instead of generic dispatch on Node is a better approach.

I guess I'm just asking to learn more of your thoughts/plans.  :)

I'm happy to r+ this patch once understanding a bit more context.

[Antti Koivisto]

It is not simpler as such. The goal is to tighten the code so that it statically describes what is possible and what is not. Beyond the architectural improvement, the end goals include

- Make it go faster. The current code is unnecessarily branchy because it is overly generic.
- Use lazy attach during parsing to reduce unnecessary style recalcs. To do this efficiently we need to change the renderer construction in ways that benefit from reduced genericity.
- Get rid of the Text nodes in common case. We should synthesize them on DOM access only. This requires separating the text construction path.

[Ojan Vafai]

(In reply to comment #4)
> It is not simpler as such. The goal is to tighten the code so that it statically describes what is possible and what is not. 

Even without these goals, I think tighening up the code is a big win. I really like the direction of this change. 

I cringe a little at the code duplication between createRendererForElementIfNeeded and createRendererForTextIfNeeded. I think you could get rid of much of it by adding a couple static helper functions?

> Beyond the architectural improvement, the end goals include
> 
> - Make it go faster. The current code is unnecessarily branchy because it is overly generic.
> - Use lazy attach during parsing to reduce unnecessary style recalcs. To do this efficiently we need to change the renderer construction in ways that benefit from reduced genericity.
> - Get rid of the Text nodes in common case. We should synthesize them on DOM access only. This requires separating the text construction path.

These are great goals! I wonder with the latter goal if we could even take it a step further and get rid of the C++ side object entirely and just have the javascript wrapper. Obviously, this would be a followup step. The big difficulty with this would be dealing with editing/selections since those can be inside Text nodes. Maybe we could have a lighter-weight thing inside editing/selection to handle text without actually creating the Nodes, but that sounds tricky.

[Antti Koivisto]

(In reply to comment #5)
> I cringe a little at the code duplication between createRendererForElementIfNeeded and createRendererForTextIfNeeded. I think you could get rid of much of it by adding a couple static helper functions?

I tried some helper functions but they didn't seem particularly helpful in the end. I think this can be simplified more by taking the refactoring bit further. This is not meant to be the end state.

[Ojan Vafai]

(In reply to comment #6)
> I tried some helper functions but they didn't seem particularly helpful in the end. I think this can be simplified more by taking the refactoring bit further. This is not meant to be the end state.

Fair enough.

[Eric Seidel (no email)]

Comment on attachment 175855 [details]
patch

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

Thank you *very* much for such a clear explanation.  And thank you again for working on this.  core rendering like this still needs a lot of love. :(

> Source/WebCore/dom/CharacterData.cpp:95
> +        toText(this)->updateTextRenderer(oldLength, 0);

This is such a horribly cryptic method name/calling-convention.  (Not your fault.  Just hope we make it clearer some day.)

> Source/WebCore/dom/Element.cpp:1102
> +    return (document()->documentElement() == this) || (context.style()->display() != NONE);

I didn't know that the body always got a render?  Interesting.

> Source/WebCore/dom/Text.cpp:311
> +    if (!textRenderer || !textRendererIsNeeded(NodeRenderingContext(this, textRenderer->style()))) {
> +        reattach();

I'm slightly confused by this re-attach here.

[Elliott Sprehn]

(In reply to comment #8)
> (From update of attachment 175855 [details])
> ...
> > Source/WebCore/dom/Element.cpp:1102
> > +    return (document()->documentElement() == this) || (context.style()->display() != NONE);
> 
> I didn't know that the body always got a render?  Interesting.

That's actually the document element so it would be the <html> element, but that's broken right now. See: https://bugs.webkit.org/show_bug.cgi?id=102975#c10

[Elliott Sprehn]

Comment on attachment 175855 [details]
patch

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

> Source/WebCore/dom/NodeRenderingContext.cpp:217
>      moveToFlowThreadIfNeeded();

You should remove isElementNode() check in moveToFlowThreadIfNeeded() since we never call it on non-elements with this patch.

> Source/WebCore/dom/NodeRenderingContext.cpp:219
> +    if (!element->rendererIsNeeded(*this)) {

Can we either rename this elementRendererIsNeeded to match the new createRendererForElementIfNeeded or leave the method name alone for createRendererIfNeeded (as you have textRendererIsNeeded and createTextRenderer)? We lose the parity in the methods from this patch.

> Source/WebCore/dom/Text.cpp:306
> +{

The old version of this method was shorter and seemed easier to understand.

[Antti Koivisto]

http://trac.webkit.org/changeset/135668

[Antti Koivisto]

> > Source/WebCore/dom/Text.cpp:311
> > +    if (!textRenderer || !textRendererIsNeeded(NodeRenderingContext(this, textRenderer->style()))) {
> > +        reattach();
> 
> I'm slightly confused by this re-attach here.

The change in text content may change the result of textRendererIsNeeded, requiring either addition or removal of the renderer. More optimal version would check if the change actually had such impact before reattaching. I suppose it has not been a real world problem so far.

[Antti Koivisto]

(In reply to comment #10)
> You should remove isElementNode() check in moveToFlowThreadIfNeeded() since we never call it on non-elements with this patch.

Done.

> Can we either rename this elementRendererIsNeeded to match the new createRendererForElementIfNeeded or leave the method name alone for createRendererIfNeeded (as you have textRendererIsNeeded and createTextRenderer)? We lose the parity in the methods from this patch.

We don't usually prefix member functions with the class type. Only reason I did it for Text was to avoid confusing these non-virtual calls with with the virtual Element functions.

> The old version of this method was shorter and seemed easier to understand.

Shortness is a non-goal and I obviously disagree with the 'easier to understand' part.