[CSS Grid Layout] Do not grow tracks when the growth factor is 0
Created attachment 237690 [details] Patch
Ping
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.
(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.
Created attachment 238111 [details] Patch
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 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
Darin could you review the patch again please? It's finally quite different to the original one.
Gentle reminder...
Committed r173868: <http://trac.webkit.org/changeset/173868>