Bug 126460 - [CSS Regions] Improve RenderFlowThread::repaintRectangleInRegions() to avoid loop over all the regions
Summary: [CSS Regions] Improve RenderFlowThread::repaintRectangleInRegions() to avoid ...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 118668
  Show dependency treegraph
 
Reported: 2014-01-03 14:26 PST by Manuel Rego Casasnovas
Modified: 2022-07-13 09:20 PDT (History)
14 users (show)

See Also:


Attachments
Patch (5.34 KB, patch)
2014-01-03 14:49 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (486.93 KB, application/zip)
2014-01-03 16:09 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (646.36 KB, application/zip)
2014-01-03 16:24 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (460.98 KB, application/zip)
2014-01-03 16:47 PST, Build Bot
no flags Details
Patch (7.55 KB, patch)
2014-01-07 15:14 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (7.56 KB, patch)
2014-01-07 15:22 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (7.56 KB, patch)
2014-01-15 12:58 PST, Manuel Rego Casasnovas
abucur: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2014-01-03 14:26:51 PST
In each RenderSelectionInfo::repaint() it's used the RenderObject::containerForRepaint(). Which in most of the situations returns the RenderNamedFlowThread for all the elements under RenderNamedFlowThread in the render tree.

This causes that for every element under RenderNamedFlowThread the method RenderFlowThread::repaintRectangleInRegions() is called. Taking a look to this method it has a loop over all the regions forcing a repaint. This means that if you have 1000 regions, even if you're just selecting in one of them, a repaint in the rest of regions is executed:
void RenderFlowThread::repaintRectangleInRegions(const LayoutRect& repaintRect, bool immediate) const
{
    if (!shouldRepaint(repaintRect) || !hasValidRegionInfo())
        return;

    LayoutStateDisabler layoutStateDisabler(&view()); // We can't use layout state to repaint, since the regions are somewhere else.

    // We can't use currentFlowThread as it is possible to have interleaved flow threads and the wrong one could be used.
    // Let each region figure out the proper enclosing flow thread.
    CurrentRenderFlowThreadDisabler disabler(&view());

    for (auto iter = m_regionList.begin(), end = m_regionList.end(); iter != end; ++iter) {
        RenderRegion* region = *iter;

        region->repaintFlowThreadContent(repaintRect, immediate);
    }
}

This could be avoided but just issuing a repaint for the regions affected by the repaintRect.
Comment 1 Manuel Rego Casasnovas 2014-01-03 14:49:03 PST
Created attachment 220340 [details]
Patch

In my machine Layout/RegionsSelection.html goes down from 1040ms to 900ms.
Comment 2 Build Bot 2014-01-03 16:09:08 PST
Comment on attachment 220340 [details]
Patch

Attachment 220340 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5429366674685952

New failing tests:
fast/regions/repaint/repaint-regions-overflow.html
Comment 3 Build Bot 2014-01-03 16:09:10 PST
Created attachment 220349 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 4 Build Bot 2014-01-03 16:24:12 PST
Comment on attachment 220340 [details]
Patch

Attachment 220340 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4578599420035072

New failing tests:
fast/regions/repaint/repaint-regions-overflow.html
Comment 5 Build Bot 2014-01-03 16:24:14 PST
Created attachment 220351 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 6 Build Bot 2014-01-03 16:47:44 PST
Comment on attachment 220340 [details]
Patch

Attachment 220340 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5164571471904768

New failing tests:
fast/regions/repaint/repaint-regions-overflow.html
Comment 7 Build Bot 2014-01-03 16:47:46 PST
Created attachment 220353 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 8 Manuel Rego Casasnovas 2014-01-07 15:14:11 PST
Created attachment 220554 [details]
Patch
Comment 9 EFL EWS Bot 2014-01-07 15:18:32 PST
Comment on attachment 220554 [details]
Patch

Attachment 220554 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/6258986852548608
Comment 10 Manuel Rego Casasnovas 2014-01-07 15:22:43 PST
Created attachment 220556 [details]
Patch
Comment 11 Manuel Rego Casasnovas 2014-01-15 12:58:34 PST
Created attachment 221299 [details]
Patch

Add improvement percentages for new perftests.
Comment 12 Brent Fulgham 2014-04-16 12:18:33 PDT
This patch looks good to me, but I think hyatt or dino should give it a review before we land it.
Comment 13 Andrei Bucur 2014-05-12 01:22:47 PDT
Comment on attachment 221299 [details]
Patch

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

> Source/WebCore/rendering/RenderFlowThread.cpp:441
> +        // If no regions intersect the repaint rect, it is in the flow thread overflow.

Unfortunately things are a bit more complex now that content can overflow any region. Imagine the case of two regions and the first region has a really tall video inside it, overflowing the first region. The controls will be at the bottom of the video. If you want to repaint them, the rectangle will "geometrically" fit inside the second region. However, because the video is unsplittable it is rendered inside the first region. So basically you will ask the second region to do the repaint when actually the first region should do it.

Such a change should also take into account the originating box or the region range of the box. This is also really tricky to do because it means we should trigger a repaint when the range of a box changes. I don't think there's an easy way to implement this optimization :(.
Comment 14 Brent Fulgham 2022-07-13 09:20:33 PDT
CSS Regions were removed in Bug 174978.