1362902 - Review choice of Variant Tag types

Status: NEW

(Core :: MFBT, enhancement)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

While working on bug 1338389, I felt the urge to change the possible Tag types (from bool/uint_fast8_t/size_t to just uint_fast8_t/uint_fast32_t), but :Waldo objected.

As that was a fly-by change unrelated to the main purpose of bug 1338389, I'm opening this new bug to continue the discussion here...


Jeff remarked in bug 1338389 comment 2:
> (In reply to Gerald Squelart [:gerald] from comment #1)
> > - Removed `bool` tag, I think it's useless.
> > - Bigger tag type is `uint_fast32_t` instead of `size_t` -- Who would give
> > more than 2^32 template arguments? :-P
> 
> Aside from nitpickiness, what's wrong with the existing code?  Add a
> uint32_fast_t variant if you must, but it seems unnecessary to muck with
> this, and you're undoing deliberate changes.

I responded:
> I see you've done that in bug 1287243.
> 
> I'm not really undoing your changes, there is still a type optimization done
> on the tag, depending on the number of Variant alternatives. I'm just
> tweaking the chosen types...
> 
> My thinking was:
> * for `bool`:
> - If uint_fast8_t is 1 byte, then `bool` doesn't bring any size benefit.
> - If uint_fast8_t is bigger than 1 byte, then `bool` can indeed be smaller,
> but it will also presumably be slower.
> - Using C++ `bool` to record something else than `true` or `false` feels
> weird to me these days. :-)
> Based on all this, I thought the `bool` option was not worth worrying about.
> But I admit I haven't studied this change in depth (generated code,
> performance...)
> 
> * for `size_t`:
> - In practice we should never encounter 2^32-argument Variant, so it seemed
> wasteful to always use size_t which can be 64 bits, when a fast 32-bit value
> should be enough, and gain a bit of space.
> 
> If bool/fast8/size_t is in fact the best choice ever, never to be changed, I
> think it'd be nice to put a comment in the code to explain that, and avoid
> another enthusiastic developer from trying to change it again in the future.
> ;-)

And upon further thoughts, I'm now thinking that we could just use uint_fast8_t in all cases, because who would really need a Variant<Ts> with more than 256 types?
(Yes, it's possible to build such an example through meta-programming, but then, would a Variant of this many MP-generated types be reasonable? And if someone really needed that, they'd certainly be able to re-add a Tag-type selector...)

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

As a quick exercise, I added a `static_assert(sizeof...(Ts) < 128, "")` locally (Mac) and nothing went wrong. :-)

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

A bit more information about integral types (and mostly to satisfy my own curiosity), here's a table of most types and their actual underlying fundamental types and sizeof's:
https://docs.google.com/a/mozilla.com/spreadsheets/d/1xhrgZ5h6ZaIObeU9hj78FeTzAX_ye0sxaQ0EISwIng4/edit?usp=sharing

In particular, notice that all the 'fast' types have the same size as their non-fast counterpart, except for fast16 which uses 32 bits.

So uint_fast8_t is currently only 1 byte on all platforms we target, effectively rendering the bool option redundant.
I'm not arguing that we shouldn't usually try to make code efficient in a generic portable manner, I'm just adding one more data point against the current bool option.
Also, the initial choice of a 'fast' type means we ultimately value speed over size; so if somewhere uint_fast8_t is bigger than 1 byte in order to be faster on some architecture, then I would argue that using a (presumably) smaller bool could possibly make our code slower with 2 types than with 3+ -- unless bool is bigger there too (but presumably not smaller than uint_fast8_t) in which case it's redundant again.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

But then, my proposal would probably end up compiling to the same binary as the current code in all current cases (i.e., 1-byte tag), so no big deal if we don't change it -- But I think it would still be nice, for slightly easier maintenance, so I'm not closing this bug.

Sorry for the noise, I was just trying to leave the place in a better state than I found it.