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
Created attachment 226311 [details] Patch
Average float_50_100 test results: ~317ms without patch vs ~287ms with patch -> ~10% better with patch (measured on efl port on pc)
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'?
Created attachment 231960 [details] Patch
(In reply to comment #4) > Created attachment 231960 [details] > Patch It seems László addressed the review comments. Brent?
ping?
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?
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.