| Summary: | [CSS Regions] contentBoxRect().location() is always 0,0 in RenderRegion | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Manuel Rego Casasnovas <rego> | ||||||||||||||||||||||||||
| Component: | CSS | Assignee: | Manuel Rego Casasnovas <rego> | ||||||||||||||||||||||||||
| Status: | RESOLVED WONTFIX | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | bfulgham, buildbot, clopez, commit-queue, esprehn+autocc, glenn, hyatt, jfernandez, kling, kondapallykalyan, mihnea, rniwa, stavila, WebkitBugTracker, zalan | ||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||
| Bug Depends on: | 118665, 119230, 124963 | ||||||||||||||||||||||||||||
| Bug Blocks: | 118668 | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
|
Description
Manuel Rego Casasnovas
2013-11-13 07:59:29 PST
Created attachment 216801 [details]
Patch
(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 Created attachment 217055 [details] Patch Rebased patch after patch for bug #118665 landed (r159337). 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%. 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? (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. 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. 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.
Created attachment 221304 [details]
Patch
Rebased against current trunk. Added improvement percentages for new perftests.
Mihnea -- now that the performance tests are working, can you confirm the performance improvement? Is this patch something we want to take? 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).
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.
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 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
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 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
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 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
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.
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..... Created attachment 231829 [details]
Patch
New version following suggestion by stavila. Thanks.
I am puzzled why contentBoxRect() has such a performance hit. It does not seem to do any (heavy) calculation at all. (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. (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.) (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. 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? 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.
CSS Regions were removed in Bug 174978. |