Bug 79591 – Avoid doing 2 hash lookups if we override RenderBox's logical width / height

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

[Julien Chaffraix]

Currently all RenderBoxes are calling clearOverrideSize. However a RenderBox could have an override size in only 3 cases:
* if it's a child of a RenderDeprecatedFlexibleBox
* if it's a child of a RenderFlexibleBox
* if it's a RenderTableCell

That would remove 2 hash lookups from RenderBox::willBeDestroyed.

[Julien Chaffraix]

Created attachment 128899 [details]
Proposed refactoring.

[Benjamin Poulain]

Comment on attachment 128899 [details]
Proposed refactoring.

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

r~ from me :-D

> Source/WebCore/rendering/RenderBox.cpp:-251
> -    clearOverrideSize();
> -

I am not convinced by this change. You are spreading complexity to avoid one very simple call.
Could you find if this has any impact?

> Source/WebCore/rendering/RenderBox.cpp:802
> -    if (hasOverrideHeight())
> +    if (gOverrideHeightMap)
>          gOverrideHeightMap->remove(this);
> -    if (hasOverrideWidth())
> +    if (gOverrideWidthMap)
>          gOverrideWidthMap->remove(this);

This looks good.

[Benjamin Poulain]

Comment on attachment 128899 [details]
Proposed refactoring.

Let's r- until you benchmark it.

[Julien Chaffraix]

> Let's r- until you benchmark it.

Thanks for doubting my change. I tested on html5-full-renderer and it looks like it regressed a bit the benchmark (around 1%, small but significant). It looks like the branches we introduced were killing us and / or my assumption about the use of RenderDeprecatedFlexibleBox or RenderFlexibleBox were wrong.

I will re-purpose the bug to center around the good part and put a new patch for review.

[Julien Chaffraix]

Re-purposing the bug on the part that is safe.

[Julien Chaffraix]

Created attachment 129089 [details]
Smaller, better change. No change on html5-full-render.html this time.

[Benjamin Poulain]

Damn review thief! ;)

[Antonio Gomes]

(In reply to comment #7)
> Damn review thief! ;)

:'( ... <:o)

[WebKit Review Bot]

Comment on attachment 129089 [details]
Smaller, better change. No change on html5-full-render.html this time.

Clearing flags on attachment: 129089

Committed r109028: <http://trac.webkit.org/changeset/109028>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.