Bug 107740 – Share code between the different min-content / max-content code paths

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

Currently the min-content / max-content implementation is copied and pasted between the different branches in resolveContentBasedTrackSizingFunctions.

To ease maintenance and match the specification more closely, all the code paths should rely on the same primitives, using some parametrization to tweak the behavior as needed.

[Julien Chaffraix]

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

[Tony Chang]

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.

[Julien Chaffraix]

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

[Tony Chang]

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

[Julien Chaffraix]

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

[Tony Chang]

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

[Julien Chaffraix]

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

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.