Bug 107740

Created attachment 184332 [details] Proposed change 1: Use some template magic to merge the different branches.

Comment on attachment 184332 [details] Proposed change 1: Use some template magic to merge the different branches. Why did you decide to use a template rather than passing in parameters and using if()s in resolveContentBasedTrackSizingFunctionsForItems? This doesn't look that performance sensitive and I think it would be easier to read the code without templates.

(In reply to comment #2) > (From update of attachment 184332 [details]) > Why did you decide to use a template rather than passing in parameters and using if()s in resolveContentBasedTrackSizingFunctionsForItems? This doesn't look that performance sensitive and I think it would be easier to read the code without templates. I could be convinced otherwise but it looked cleaner to use a template for the following reasons: - Less arguments: we already have a lot of arguments and adding 2 more is going to add more pain. - If you pass the Accumulator as an argument, you need to pass an instance of it - meaning the need for a base class and more boiler plate code - or 2 function pointers - even *more* arguments. - Code reuse: I am pretty sure we will need to re-use distributeSpaceToTracks in resolveContentBasedTrackSizingFunctionsForItems. The 2 previous argument-based solutions would yield to more duplication or if()'s. - Less branches: from a readability perspective, what you are proposing will yield to lots of if()'s. - Conceptually, it's really a core algorithm that you specialize using some trait class (see the HashTraits / VectorTraits in WTF for example).

Comment on attachment 184332 [details] Proposed change 1: Use some template magic to merge the different branches. View in context: https://bugs.webkit.org/attachment.cgi?id=184332&action=review I still think using parameters would be easier to read. This is my proposal: Make an Accumulator interface and have MaxBreadthAccumulator and UsedBreadthAccumulator inherit from it. You could CREATE_STATIC_LOCAL them in resolveContentBasedTrackSizingFunctionsForItems, although stack allocating doesn't seem like a big deal to me. Pass in an enum for which Accumulator you want to use. In the beginning of resolveContentBasedTrackSizingFunctionsForItems use a ternary to set a locale reference to the right one. MinContentFunction / MaxContentFunction is already just an enum with a ternary in the code. Switching it to a parameter would not change the function body, although it would be an extra branch at runtime. The result would be the 2 template parameters are now function parameters. They're enums, so it's that different to read and you'll get nice compiler warnings if you misplace them (same as template params). This adds 1 new ternary branch at the beginning of resolveContentBasedTrackSizingFunctionsForItems for the accumulator and there will be virtual dispatch for the methods on the accumulator. If you have some data suggesting that this is going to be the bottleneck during layout (e.g., if you had n grid cells, this will be run n^2 times), I could probably be convinced into turning this into a template parameter. In Flexbox, we were worried that we would spend a lot of time in flex layout and would try to avoid looping over the children. We added some perf tests and it turned out that looping over your children is way faster than say, layout out some text. > Source/WebCore/rendering/RenderGrid.cpp:63 > + static LayoutUnit value(GridTrack& track) { return track.m_usedBreadth; } Nit: const GridTrack& track

(In reply to comment #4) > (From update of attachment 184332 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184332&action=review > > I still think using parameters would be easier to read. This is my proposal: > > Make an Accumulator interface and have MaxBreadthAccumulator and UsedBreadthAccumulator inherit from it. You could CREATE_STATIC_LOCAL them in resolveContentBasedTrackSizingFunctionsForItems, although stack allocating doesn't seem like a big deal to me. > > Pass in an enum for which Accumulator you want to use. In the beginning of resolveContentBasedTrackSizingFunctionsForItems use a ternary to set a locale reference to the right one. > > MinContentFunction / MaxContentFunction is already just an enum with a ternary in the code. Switching it to a parameter would not change the function body, although it would be an extra branch at runtime. > > The result would be the 2 template parameters are now function parameters. They're enums, so it's that different to read and you'll get nice compiler warnings if you misplace them (same as template params). Your proposal really feels like a poor man's version of the template: we still pass more parameters and need more boiler-plate code (base class + inheritance, some static instance or stack allocations). We may as well just pass function pointers in this case as it would remove the need for much of the boiler-plate code. > This adds 1 new ternary branch at the beginning of resolveContentBasedTrackSizingFunctionsForItems for the accumulator and there will be virtual dispatch for the methods on the accumulator. If you have some data suggesting that this is going to be the bottleneck during layout (e.g., if you had n grid cells, this will be run n^2 times), I could probably be convinced into turning this into a template parameter. > > In Flexbox, we were worried that we would spend a lot of time in flex layout and would try to avoid looping over the children. We added some perf tests and it turned out that looping over your children is way faster than say, layout out some text. I totally agree that performance claims should be based on data but I wasn't making any. The goal was to be pragmatic and use the best tool for the job as templates: * don't create as much boiler-plate code as a class hierarchy would. * don't add at least 3 parameters to the functions that starts to have too many arguments as a function pointers' approach. You also have to think about what's coming: distributeSpaceToTracks requires 5 arguments in the specification, which could easily end up being 8+ arguments if we are not careful.

(In reply to comment #5) > The goal was to be pragmatic and use the best tool for the job as templates: > * don't create as much boiler-plate code as a class hierarchy would. > * don't add at least 3 parameters to the functions that starts to have too many arguments as a function pointers' approach. You also have to think about what's coming: distributeSpaceToTracks requires 5 arguments in the specification, which could easily end up being 8+ arguments if we are not careful. It seems like we could have the same number of arguments as the template version, it's just a question of whether it's a template argument or a function argument. Also, it would be fine to create a struct to hold related arguments and pass that as an argument to make it easier to read like we do in other parts of the code.

Created attachment 184813 [details] Proposed change v2: Use function pointers instead.

Comment on attachment 184813 [details] Proposed change v2: Use function pointers instead. Clearing flags on attachment: 184813 Committed r140894: <http://trac.webkit.org/changeset/140894>

All reviewed patches have been landed. Closing bug.