Bug 124281 – [CSS Regions] contentBoxRect().location() is always 0,0 in RenderRegion

• Component: CSS
• Status: RESOLVED WONTFIX

[Manuel Rego Casasnovas]

Every time a repaint is done in a RenderRegion contentBoxRect().location is calculated again.

During the selection a repaint is required a lot of times so this affects negatively to the performance of the selection in documents with CSS Regions.

[Manuel Rego Casasnovas]

Created attachment 216801 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to comment #1)
> Created an attachment (id=216801) [details]
> Patch

Just for reference in my machine these are the results of running the performance test Layout/RegionsSelection.html (uploaded to bug #119230) with and without this patch:
* Without this patch: 5178.882 ms
* With this patch: 3830.194 ms

Also you can do a quick test with the following HTML example and check the time required to do the selection in the JavaScript console:
http://people.igalia.com/mrego/css/perf/test-regions.html

Again the results in my machine are:
* Without this patch: 186.133 ms
* With this patch: 106.979 ms

[Manuel Rego Casasnovas]

Created attachment 217055 [details]
Patch

Rebased patch after patch for bug #118665 landed (r159337).

[Manuel Rego Casasnovas]

Created attachment 217532 [details]
Patch

In my machine this patch provides a 5% better result for Layout/RegionsSelection.html perftest. Also checking the results with 1000 regions in https://rawgithub.com/Igalia/css-regions-selection/master/perf-regions.html the improvement is ~14%.

[Mihnea Ovidenie]

Comment on attachment 217532 [details]
Patch

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

r-
I am not convinced that this patch provides the claimed performance improvement. I ran several times on my machine the performance test you mention, i even changed the number of regions to be 1000 instead of 100. The difference between result - with and without the patch - were in the range of 1% which in this case is inside the deviation.
I think you should come up with a different test that would show an improvement.

> Source/WebCore/ChangeLog:13
> +        It improves performance of selection in CSS Regions.

Please provide numbers and the performance tests that were run in this regard.

> Source/WebCore/ChangeLog:21
> +        * rendering/RenderRegion.h: Add new member m_regionLocation.

Should we add an accessor function to be used in other places in code where contentBoxRect().location() is used for a region?

[Manuel Rego Casasnovas]

(In reply to comment #5)
> (From update of attachment 217532 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217532&action=review
> 
> r-
> I am not convinced that this patch provides the claimed performance improvement. I ran several times on my machine the performance test you mention, i even changed the number of regions to be 1000 instead of 100. The difference between result - with and without the patch - were in the range of 1% which in this case is inside the deviation.
> I think you should come up with a different test that would show an improvement.

There's an issue with the perftest that is not doing any selection in Mac platform. I've fixed it in bug #124963 once it lands I'll update this patch with the new values (in a macmini I'm having a ~3% improvement with 100 regions).

> > Source/WebCore/ChangeLog:13
> > +        It improves performance of selection in CSS Regions.
> 
> Please provide numbers and the performance tests that were run in this regard.

Ok I'd do it, but this depends on the platform and the machine where you're testing so it's not very accurate.

> > Source/WebCore/ChangeLog:21
> > +        * rendering/RenderRegion.h: Add new member m_regionLocation.
> 
> Should we add an accessor function to be used in other places in code where contentBoxRect().location() is used for a region?

Once I provide a new patch I'll add the accessor function and use it in other parts of the code when it makes sense.

[Manuel Rego Casasnovas]

Created attachment 218352 [details]
Patch

Once perftest is fixed for Mac platform (see bug #124963) I hope now the performance gain also happens there. I'm uploading a new version of the patch with updated results and adding an accessor function as suggested in the review.

[Manuel Rego Casasnovas]

Created attachment 219909 [details]
Patch

While debugging the value of regionLocation in previous patch it was detected that it's always 0,0. Removing the usage of it in RenderRegion.

[Manuel Rego Casasnovas]

Created attachment 221304 [details]
Patch

Rebased against current trunk. Added improvement percentages for new perftests.

[Brent Fulgham]

Mihnea -- now that the performance tests are working, can you confirm the performance improvement? Is this patch something we want to take?

[Andrei Bucur]

Comment on attachment 221304 [details]
Patch

r=me

Before landing please update the performance improvements numbers and make sure the new-multi col tests don't fail (column spans where implemented in the meantime).

[Manuel Rego Casasnovas]

Created attachment 231797 [details]
Patch for landing

Patch rebased and updated perftests results. Multi-column tests are passing here in GTK+ but let's see what EWSs say.

[Build Bot]

Comment on attachment 231797 [details]
Patch for landing

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

New failing tests:
fast/repaint/spanner-with-margin.html

[Build Bot]

Created attachment 231800 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231797 [details]
Patch for landing

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

New failing tests:
fast/repaint/spanner-with-margin.html

[Build Bot]

Created attachment 231802 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 231797 [details]
Patch for landing

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

New failing tests:
fast/repaint/spanner-with-margin.html

[Build Bot]

Created attachment 231803 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Manuel Rego Casasnovas]

Created attachment 231827 [details]
Patch

Previous patch was breaking on multi-column test, now it should be fixed. abucur could you take a new look? Thanks.

[Radu Stavila]

Comment on attachment 231827 [details]
Patch

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

> Source/WebCore/rendering/RenderRegion.cpp:267
> +    LayoutPoint location = regionLocation ? *regionLocation : LayoutPoint();

Seems like you're defaulting to using LayoutPoint(0, 0) when regionLocation is nullptr. As such, considering the added risk a pointer brings compared to a reference, I think it would be best to stick to using LayoutPoint& and make LayoutPoint() its default value.

> Source/WebCore/rendering/RenderRegion.h:148
> +    void repaintFlowThreadContentRectangle(const LayoutRect& repaintRect, const LayoutRect& flowThreadPortionRect, const LayoutPoint* regionLocation = 0, const LayoutRect* flowThreadPortionClipRect = 0);

I think this would be better:

... const LayoutPoint& regionLocation = LayoutPoint(), const LayoutRect.....

[Manuel Rego Casasnovas]

Created attachment 231829 [details]
Patch

New version following suggestion by stavila. Thanks.

[zalan]

I am puzzled why contentBoxRect() has such a performance hit. It does not seem to do any (heavy) calculation at all.

[Manuel Rego Casasnovas]

(In reply to comment #22)
> I am puzzled why contentBoxRect() has such a performance hit. It does not seem to do any (heavy) calculation at all.

I was using perf profiler in selection perftests and I got the following callgraph:

-   4.85%  lt-WebKitWebPro  libwebkit2gtk-3.0.so.25.5.0         [.] WebCore::LayoutUnit::rawValue() const 
   - WebCore::LayoutUnit::rawValue() const                                                                                                    
      - 33.20% WebCore::operator-(WebCore::LayoutUnit const&, WebCore::LayoutUnit const&)                  
         - 53.64% WebCore::operator-(WebCore::LayoutUnit const&, int)                                                         
            - 53.39% WebCore::RenderBox::clientWidth() const                                                                          
               - WebCore::RenderBox::contentWidth() const                                                                                 
                  - 89.22% WebCore::RenderBox::contentBoxRect() const                                                             
                       WebCore::RenderRegion::repaintFlowThreadContent(WebCore::LayoutRect const&, bool) const  
                       WebCore::RenderFlowThread::repaintRectangleInRegions(WebCore::LayoutRect const&, bool) const  
                     - WebCore::RenderObject::repaintUsingContainer(WebCore::RenderLayerModelObject const*, WebCore::IntRect const&, bool) const
                        - 75.75% WebCore::RenderSelectionInfo::repaint()                                                                        
                             WebCore::RenderView::setSelection(WebCore::RenderObject*, int, WebCore::RenderObject*, int, WebCore::RenderView::SelectionRepaintMode)       
                             WebCore::FrameSelection::updateAppearance()                       
                             WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, unsigned int, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)   
                             WebCore::FrameSelection::moveTo(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::EUserTriggered)             
                             WebCore::DOMSelection::setBaseAndExtent(WebCore::Node*, int, WebCore::Node*, int, int&)     

As you can see contextBoxRect() does some operations over LayoutUnits:
    LayoutRect contentBoxRect() const { return LayoutRect(borderLeft() + paddingLeft(), borderTop() + paddingTop(), contentWidth(), contentHeight()); }

As it was always 0,0 in RenderRegion we can avoid this call and the perftests are improving at least on GTK+ port but I expect that they'll improve in mac too.

[zalan]

(In reply to comment #23)
> (In reply to comment #22)
> > I am puzzled why contentBoxRect() has such a performance hit. It does not seem to do any (heavy) calculation at all.
> 
> I was using perf profiler in selection perftests and I got the following callgraph:
> 
> -   4.85%  lt-WebKitWebPro  libwebkit2gtk-3.0.so.25.5.0         [.] WebCore::LayoutUnit::rawValue() const 
>    - WebCore::LayoutUnit::rawValue() const                                                                                                    
>       - 33.20% WebCore::operator-(WebCore::LayoutUnit const&, WebCore::LayoutUnit const&)                  
>          - 53.64% WebCore::operator-(WebCore::LayoutUnit const&, int)                                                         
>             - 53.39% WebCore::RenderBox::clientWidth() const                                                                          
>                - WebCore::RenderBox::contentWidth() const                                                                                 
>                   - 89.22% WebCore::RenderBox::contentBoxRect() const                                                             
>                        WebCore::RenderRegion::repaintFlowThreadContent(WebCore::LayoutRect const&, bool) const  
>                        WebCore::RenderFlowThread::repaintRectangleInRegions(WebCore::LayoutRect const&, bool) const  
>                      - WebCore::RenderObject::repaintUsingContainer(WebCore::RenderLayerModelObject const*, WebCore::IntRect const&, bool) const
>                         - 75.75% WebCore::RenderSelectionInfo::repaint()                                                                        
>                              WebCore::RenderView::setSelection(WebCore::RenderObject*, int, WebCore::RenderObject*, int, WebCore::RenderView::SelectionRepaintMode)       
>                              WebCore::FrameSelection::updateAppearance()                       
>                              WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, unsigned int, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)   
>                              WebCore::FrameSelection::moveTo(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::EUserTriggered)             
>                              WebCore::DOMSelection::setBaseAndExtent(WebCore::Node*, int, WebCore::Node*, int, int&)     
> 
> As you can see contextBoxRect() does some operations over LayoutUnits:
>     LayoutRect contentBoxRect() const { return LayoutRect(borderLeft() + paddingLeft(), borderTop() + paddingTop(), contentWidth(), contentHeight()); }
> 
> As it was always 0,0 in RenderRegion we can avoid this call and the perftests are improving at least on GTK+ port but I expect that they'll improve in mac too.
LayoutUnit is the base unit for all layout operations. If any LayoutUnit operation has a performance hit, then we need to look at it really closely. (operator- simply subtracts two integers wrapped in LayoutUnits, so not sure where the hit is.)

[Manuel Rego Casasnovas]

(In reply to comment #24)
> (In reply to comment #23)
> > As you can see contextBoxRect() does some operations over LayoutUnits:
> >     LayoutRect contentBoxRect() const { return LayoutRect(borderLeft() + paddingLeft(), borderTop() + paddingTop(), contentWidth(), contentHeight()); }
> > 
> > As it was always 0,0 in RenderRegion we can avoid this call and the perftests are improving at least on GTK+ port but I expect that they'll improve in mac too.
> LayoutUnit is the base unit for all layout operations. If any LayoutUnit operation has a performance hit, then we need to look at it really closely. (operator- simply subtracts two integers wrapped in LayoutUnits, so not sure where the hit is.)

This patch is about calling contentBoxRect() in RenderRegion when it's not needed. So, even if contentBoxRect() was super fast, if it's not needed we shouldn't call it.

On the other hand, I agree that further investigation might be done in contentBoxRect() to check where is the bottleneck causing this performance issues. However, IMHO this would be part of a different bug.

[Darin Adler]

Comment on attachment 231829 [details]
Patch

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

> Source/WebCore/ChangeLog:24
> +        It improves the results in CSS Regions perftets in the following
> +        percentages:
> +        - Layout/RegionsAuto: ~11%
> +        - Layout/RegionsAutoMaxHeight: ~17%
> +        - Layout/RegionsExtendingSelectionMixedContent: ~5%
> +        - Layout/RegionsFixed: ~21%
> +        - Layout/RegionsFixedShort: ~21%
> +        - Layout/RegionsSelectAllMixedContent: ~17%
> +        - Layout/RegionsSelection: ~4%

Wow, how does this make things so much faster? Is this code super-hot?

[Michael Catanzaro]

Comment on attachment 231829 [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.

[Brent Fulgham]

CSS Regions were removed in Bug 174978.