WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
124281
[CSS Regions] contentBoxRect().location() is always 0,0 in RenderRegion
https://bugs.webkit.org/show_bug.cgi?id=124281
Summary
[CSS Regions] contentBoxRect().location() is always 0,0 in RenderRegion
Manuel Rego Casasnovas
Reported
2013-11-13 07:59:29 PST
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.
Attachments
Patch
(3.41 KB, patch)
2013-11-13 08:09 PST
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(3.37 KB, patch)
2013-11-15 09:14 PST
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(3.22 KB, patch)
2013-11-21 01:42 PST
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(4.63 KB, patch)
2013-12-03 15:14 PST
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(6.10 KB, patch)
2013-12-23 05:21 PST
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(6.29 KB, patch)
2014-01-15 13:24 PST
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch for landing
(6.18 KB, patch)
2014-05-20 15:55 PDT
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
(466.25 KB, application/zip)
2014-05-20 16:53 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
(516.11 KB, application/zip)
2014-05-20 17:21 PDT
,
Build Bot
no flags
Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
(568.47 KB, application/zip)
2014-05-20 17:58 PDT
,
Build Bot
no flags
Details
Patch
(6.52 KB, patch)
2014-05-21 07:39 PDT
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Patch
(3.97 KB, patch)
2014-05-21 08:07 PDT
,
Manuel Rego Casasnovas
no flags
Details
Formatted Diff
Diff
Show Obsolete
(8)
View All
Add attachment
proposed patch, testcase, etc.
Manuel Rego Casasnovas
Comment 1
2013-11-13 08:09:09 PST
Created
attachment 216801
[details]
Patch
Manuel Rego Casasnovas
Comment 2
2013-11-13 08:23:03 PST
(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
Comment 3
2013-11-15 09:14:35 PST
Created
attachment 217055
[details]
Patch Rebased patch after patch for
bug #118665
landed (
r159337
).
Manuel Rego Casasnovas
Comment 4
2013-11-21 01:42:26 PST
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 5
2013-11-25 07:53:52 PST
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
Comment 6
2013-11-28 02:02:04 PST
(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
Comment 7
2013-12-03 15:14:18 PST
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
Comment 8
2013-12-23 05:21:49 PST
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
Comment 9
2014-01-15 13:24:59 PST
Created
attachment 221304
[details]
Patch Rebased against current trunk. Added improvement percentages for new perftests.
Brent Fulgham
Comment 10
2014-04-16 12:21:08 PDT
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 11
2014-05-12 01:41:44 PDT
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
Comment 12
2014-05-20 15:55:00 PDT
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 13
2014-05-20 16:53:23 PDT
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
Comment 14
2014-05-20 16:53:28 PDT
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 15
2014-05-20 17:21:07 PDT
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
Comment 16
2014-05-20 17:21:14 PDT
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 17
2014-05-20 17:58:44 PDT
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
Comment 18
2014-05-20 17:58:50 PDT
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
Comment 19
2014-05-21 07:39:21 PDT
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 20
2014-05-21 07:51:28 PDT
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
Comment 21
2014-05-21 08:07:46 PDT
Created
attachment 231829
[details]
Patch New version following suggestion by stavila. Thanks.
zalan
Comment 22
2014-05-21 10:49:45 PDT
I am puzzled why contentBoxRect() has such a performance hit. It does not seem to do any (heavy) calculation at all.
Manuel Rego Casasnovas
Comment 23
2014-05-21 12:32:43 PDT
(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
Comment 24
2014-05-21 12:46:44 PDT
(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
Comment 25
2014-05-21 14:39:04 PDT
(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 26
2014-05-24 19:32:53 PDT
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 27
2016-09-17 07:15:49 PDT
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
Comment 28
2022-07-13 09:21:19 PDT
CSS Regions were removed in
Bug 174978
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug