Make RenderElement the CachedImageClient, rather than RenderObject

• Component: New Bugs
• Status: NEW ---

[Simon Fraser (smfr)]

Make RenderElement the CachedImageClient, rather than RenderObject

[Simon Fraser (smfr)]

Created attachment 258463 [details]
Patch for EWS

[Andreas Kling]

Does this regress the sizeof(RenderElement) due to extra vtable ptr?

[Simon Fraser (smfr)]

Created attachment 258571 [details]
Patch

[Simon Fraser (smfr)]

Before:

sizeof RenderObject is 48
sizeof RenderText is 88
sizeof RenderElement is 72
sizeof RenderBoxModelObject is 80
sizeof RenderBox is 136

After:

sizeof RenderObject is 48
sizeof RenderText is 88
sizeof RenderElement is 88
sizeof RenderBoxModelObject is 96
sizeof RenderBox is 152

Hrmm, maybe we can improve packing into RenderObject to eliminate this change.

[Darin Adler]

Comment on attachment 258571 [details]
Patch

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

Not saying review+ yet because of the question in HTMLImageElement::width.

> Source/WebCore/html/HTMLImageElement.cpp:290
> -            return m_imageLoader.image()->imageSizeForRenderer(renderer(), 1.0f).width();
> +            return m_imageLoader.image()->imageSizeForRenderer(nullptr, 1.0f).width();

Change log doesn’t make clear why we want to pass nullptr here instead of renderer.

> Source/WebCore/page/EventHandler.cpp:1446
> +        RenderElement* renderElement = is<RenderElement>(renderer) ? downcast<RenderElement>(renderer) : (renderer->isText() ? renderer->parent() : nullptr);

Why is the isText check needed? Which non-RenderElement, non-text RenderObject has a requirement that we use null instead of a pointer to the object’s parent?

[Darin Adler]

(In reply to comment #4)
> Before:
> 
> sizeof RenderObject is 48
> sizeof RenderText is 88
> sizeof RenderElement is 72
> sizeof RenderBoxModelObject is 80
> sizeof RenderBox is 136
> 
> After:
> 
> sizeof RenderObject is 48
> sizeof RenderText is 88
> sizeof RenderElement is 88
> sizeof RenderBoxModelObject is 96
> sizeof RenderBox is 152
> 
> Hrmm, maybe we can improve packing into RenderObject to eliminate this
> change.

That’s bizarre. Why on earth do we see that big increase in RenderBox? I suspect the problem is that this makes it multiple inheritance and before it was single inheritance.

[Antti Koivisto]

This adds vptr to RenderElement, the results are exactly as expected.

[Antti Koivisto]

A good strategy might be to have a separate non-renderer object that is a CachedImageClient and keep those in a global RenderElement->ImageClient map. The client object would only be constructed when needed. Most renderers don't load any images.

[Antti Koivisto]

RenderElementImageLoader or something.

[zalan]

(In reply to comment #8)
> A good strategy might be to have a separate non-renderer object that is a
> CachedImageClient and keep those in a global RenderElement->ImageClient map.
> The client object would only be constructed when needed. Most renderers
> don't load any images.
I'd like to see something like that!

[Simon Fraser (smfr)]

Comment on attachment 258571 [details]
Patch

Sounds like a plan.

[Antti Koivisto]

I thought we already had static_asserts against growth of common renderers. We should add some!