Bug 99646 – Use virtual dispatch to create ContentData renderers

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Elliott Sprehn]

Use virtual dispatch to create ContentData renderers

[Elliott Sprehn]

Created attachment 169277 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 169277 [details]
Patch

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

LGTM.

> Source/WebCore/rendering/style/ContentData.cpp:88
> +    RenderObject* renderer = new (doc->renderArena()) RenderTextFragment(doc /* anonymous object */, m_text.impl());

I don't think the /* anonymous object */ comment is particularly helpful.

[Elliott Sprehn]

Created attachment 169281 [details]
Patch for landing

[Elliott Sprehn]

Comment on attachment 169277 [details]
Patch

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

>> Source/WebCore/rendering/style/ContentData.cpp:88
>> +    RenderObject* renderer = new (doc->renderArena()) RenderTextFragment(doc /* anonymous object */, m_text.impl());
> 
> I don't think the /* anonymous object */ comment is particularly helpful.

Removed.

[WebKit Review Bot]

Comment on attachment 169281 [details]
Patch for landing

Clearing flags on attachment: 169281

Committed r131666: <http://trac.webkit.org/changeset/131666>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 169281 [details]
Patch for landing

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

> Source/WebCore/ChangeLog:15
> +        Previously we switched over the StyleContentType which required a
> +        case for CONTENT_NONE and made the code look like it could return
> +        null, but this case is actually impossible because no ContentData

Don't we usually go in the opposite direction, de-virtualizing calls for better performance? A switch has better jump prediction than virtual dispatch.

I didn't look at this specific case deeply, but this makes me wonder what kind of performance testing was performed.

[Elliott Sprehn]

(In reply to comment #7)
> (From update of attachment 169281 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169281&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        Previously we switched over the StyleContentType which required a
> > +        case for CONTENT_NONE and made the code look like it could return
> > +        null, but this case is actually impossible because no ContentData
> 
> Don't we usually go in the opposite direction, de-virtualizing calls for better performance? A switch has better jump prediction than virtual dispatch.
> 
> I didn't look at this specific case deeply, but this makes me wonder what kind of performance testing was performed.

There was already a virtual call in there from type() which is what the switch was over. I didn't do any perf testing on this because I can't imagine that having no switch and the same number of virtual calls is slower. :)

See the "switch (content->type()) {" that was deleted.

[Alexey Proskuryakov]

Thank you for the explanation.