Bug 136575 – [CSS Grid Layout] Do not grow tracks when the growth factor is 0

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[CSS Grid Layout] Do not grow tracks when the growth factor is 0

[Sergio Villar Senin]

Created attachment 237690 [details]
Patch

[Sergio Villar Senin]

Ping

[Darin Adler]

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.

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

Created attachment 238111 [details]
Patch

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

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

[Sergio Villar Senin]

Darin could you review the patch again please? It's finally quite different to the original one.

[Sergio Villar Senin]

Gentle reminder...

[Sergio Villar Senin]

Committed r173868: <http://trac.webkit.org/changeset/173868>