Bug 136575 - [CSS Grid Layout] Do not grow tracks when the growth factor is 0
Summary: [CSS Grid Layout] Do not grow tracks when the growth factor is 0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords:
Depends on:
Blocks: 60731 136940
  Show dependency treegraph
 
Reported: 2014-09-05 07:12 PDT by Sergio Villar Senin
Modified: 2014-10-15 04:44 PDT (History)
9 users (show)

See Also:


Attachments
Patch (2.43 KB, patch)
2014-09-05 07:31 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (4.41 KB, patch)
2014-09-15 03:11 PDT, Sergio Villar Senin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2014-09-05 07:12:17 PDT
[CSS Grid Layout] Do not grow tracks when the growth factor is 0
Comment 1 Sergio Villar Senin 2014-09-05 07:31:22 PDT
Created attachment 237690 [details]
Patch
Comment 2 Sergio Villar Senin 2014-09-12 06:44:41 PDT
Ping
Comment 3 Darin Adler 2014-09-12 07:24:11 PDT
Comment on attachment 237690 [details]
Patch

If this really makes such a significant difference, there are a lot of other things you should consider to make this code faster. The most obvious one would be to consider if "track.m_maxBreadth - trackBreadth" is usually 0 and if so, skip the availableLogicalSpaceShare computation entirely.

It seems bizarre that this would be so hot that adding the branch just to avoid some addition and subtraction operations would make a measurable difference.
Comment 4 Sergio Villar Senin 2014-09-15 02:28:54 PDT
(In reply to comment #3)
> (From update of attachment 237690 [details])
> If this really makes such a significant difference, there are a lot of other things you should consider to make this code faster. The most obvious one would be to consider if "track.m_maxBreadth - trackBreadth" is usually 0 and if so, skip the availableLogicalSpaceShare computation entirely.
> 
> It seems bizarre that this would be so hot that adding the branch just to avoid some addition and subtraction operations would make a measurable difference.

That shocked me too, there seem no be no clear reason why the difference is so significant, but it was there when I tried the patch. I gave it another run today, and it seems that the difference is no longer there, I'm not getting significant improvements (+0.6% at the most) anymore so I think I'm abandoning this.
Comment 5 Sergio Villar Senin 2014-09-15 03:11:38 PDT
Created attachment 238111 [details]
Patch
Comment 6 Sergio Villar Senin 2014-09-15 05:28:25 PDT
So in the end I gave it another shot and decided to implement the improvement you suggested. That resulted in a ~4% gain which was great, but then I realized that we could avoid a lot of useless work when there is no extra space to distribute. So I got a compact patch that gives us 16% of improvement (even 22% in some machines) in the auto-grid-lots-of-data.html perf test case. Obviously for real world use cases the improvement wouldn't likely be that huge but it's a good step forward IMO.
Comment 7 Sergio Villar Senin 2014-09-15 08:11:22 PDT
Comment on attachment 238111 [details]
Patch

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

> Source/WebCore/rendering/RenderGrid.cpp:605
> +            LayoutUnit growthShare = std::min(availableLogicalSpaceShare, track.m_maxBreadth - trackBreadth);

This should be obviously std::min(availableLogicalSpaceShare, trackGrowthPotential), will fix it before landing if r+'ed
Comment 8 Sergio Villar Senin 2014-09-16 01:10:32 PDT
Darin could you review the patch again please? It's finally quite different to the original one.
Comment 9 Sergio Villar Senin 2014-09-22 08:49:35 PDT
Gentle reminder...
Comment 10 Sergio Villar Senin 2014-09-23 01:37:24 PDT
Committed r173868: <http://trac.webkit.org/changeset/173868>