Bug 122815 – Optimize RenderBlock::lowestFloatLogicalBottom by caching previously calculated values

• Component: Layout and Rendering
• Status: NEW

[Ryosuke Niwa]

Consider merging https://chromium.googlesource.com/chromium/blink/+/b0ba9a1ad19d5519cb03c3d42b38c3b8976e855e

This CL moves RenderBlock::lowestFloatLogicalBottom into FloatingObjects, and introduces a small cache to track the previously calculated lowestFloatLogicalBottom for each FloatingObject::type.

On my z620, this is ~25% faster on blink_perf's float_50_100 benchmark, and ~30% faster on android-gn. On oilevent from tough_layout_cases, this is about ~10% improvement.

Linux: 

Without patch: 
Avg floats_50_100: 255.006988ms 
Sd floats_50_100: 6.590191ms 

With patch: 
Avg floats_50_100: 189.995312ms 
Sd floats_50_100: 4.420704ms 


Android: 

Without patch: 
Avg floats_50_100: 6374.133600ms 
Sd floats_50_100: 852.598792ms 

With patch: 
Avg floats_50_100: 4205.389400ms 
Sd floats_50_100: 659.635617ms

[Laszlo Vidacs]

Created attachment 226311 [details]
Patch

[Laszlo Vidacs]

Average float_50_100 test results: ~317ms without patch vs ~287ms with patch -> ~10% better with patch (measured on efl port on pc)

[Brent Fulgham]

Comment on attachment 226311 [details]
Patch

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

This looks like a good change, but I'd like some confirmation that we really want to switch from using an '&' test to the equality. Also, the naming scheme you've switched to is not acceptable.

> Source/WebCore/rendering/FloatingObjects.cpp:302
> +        for (FloatingObjectSet::const_iterator it = floatingObjectSet.begin(); it != end; ++it) {

This would read better as an 'auto'. The original code this is replacing was using auto previously.

> Source/WebCore/rendering/FloatingObjects.cpp:303
> +            const FloatingObject* r = it->get();

'floatingObject' is a much better name than 'r', and is what was used in the code you are replacing.

> Source/WebCore/rendering/FloatingObjects.cpp:317
> +        for (FloatingObjectSet::const_iterator it = floatingObjectSet.begin(); it != end; ++it) {

Ditto above (auto).

> Source/WebCore/rendering/FloatingObjects.cpp:318
> +            const FloatingObject* r = it->get();

Ditto above ('floatingObject' name).

> Source/WebCore/rendering/FloatingObjects.cpp:319
> +            if (r->isPlaced() && r->type() == floatType)

We used to use an '& floatType' test here. Why do we want equality now? Do we no longer need to handle the case where floatingObject->type() is a superset of types that encompasses 'floatType'?

[Laszlo Vidacs]

Created attachment 231960 [details]
Patch

[Csaba Osztrogonác]

(In reply to comment #4)
> Created attachment 231960 [details]
> Patch

It seems László addressed the review comments. Brent?

[Csaba Osztrogonác]

ping?

[Simon Fraser (smfr)]

Comment on attachment 231960 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Introduce a small cache to track previously calculated lowestFloatLogicalBottom values for FloatingObjects.
> +        Based on blink issue https://chromiumcodereview.appspot.com/23364008 by shatch.

What pages benefit from this cache? What is the memory cost? Can we add a performance test?

[Michael Catanzaro]

Comment on attachment 231960 [details]
Patch

Hi,

Apologies that your patch was not reviewed in a timely manner. Since it's now quite old, I am removing it from the review request queue. Please consider rebasing it on trunk and resubmitting.

To increase the chances of getting a review, consider using 'Tools/Scripts/webkit-patch upload --suggest-reviewers' to CC reviewers who might be interested in this bug.