These give some very, very nice advantages: 1) Strong typing -- no more implicit usage as an integer. 2) Forward declaration -- this should allow us to hide all the internal (and external) trace kinds. 3) Specified size -- we no longer have to declare these as int in the header and cast everywhere in order to have predictable layouts.

I actually did this for AllocKind a while back, but it was a pain in the ass and didn't seem all that useful. I can probably rebase it more easily than doing it from scratch if we want it though :) The annoying thing about the strong typing is that you can't use them as array indices without a cast, which adds a fair bit of noise all over.

Assuming this is one of the GC enums you'd like to see converted, something like this :) (*) I went with size_t for the type as the usual cross-platform default. I don't know if we have any specific wishes for this, although CompactFreeSpan requires them to fit in 8 bits, so uint8_t would also make sense. (*) I removed the FINALIZE_ prefix since everywhere will now have AllocKind:: instead. (*) I went with ALLOCKIND_LIMIT and ALLOCKIND_OBJECT_LIMIT for the upper bounds. Perhaps ALLOC_KIND_LIMIT would make more sense, or even something that drops the all caps. This patch is pretty mechanical for the most part, although I took the liberty to change some for loop headers to use < rather than != and prefix-++ rather than postfix-++. The above points should be easy enough to change with the patch in hand. I also changed a few instances of FINALIZE_* in comments, and similarly s/finalize kind/alloc kind/, except where 'finalization' made more sense in context.

Comment on attachment 8574290 [details] [diff] [review] Convert js::gc::AllocKind to an enum class. Review of attachment 8574290 [details] [diff] [review]: ----------------------------------------------------------------- This is great, thanks! I'd give it an unequivocal r+ except for the size_t pollution: can you try the suggestion I added below for removing the new casts? If it works out as easily as I think it will, I'd much prefer to land in that form. On the other hand, if it does turn out to be a major hassle, I'd definitely still like to land this as is -- it's enough new line noise, however, that I think it's worth making a bit of extra effort to clean up first. ::: js/src/gc/Heap.h @@ +74,5 @@ > TenuredHeap > }; > > /* The GC allocation kinds. */ > +enum class AllocKind : size_t { I think uint8_t is probably the right type for this; we impose this requirement manually anyway, so we might as well embed it in the type system. @@ +677,1 @@ > } For a different bug, probably 1139542, We should make the thing-size and first-thing-offset mapping a static template expansion of the kind. ::: js/src/jsgc.cpp @@ +1786,5 @@ > > inline void > ArenaLists::prepareForIncrementalGC(JSRuntime *rt) > { > + for (size_t i = 0; i < ALLOCKIND_LIMIT; ++i) { For a separate patch (unless you feel adventurous): It would be nice to replace ALLOCKIND_LIMIT based iteration with a range-based iteration. Something like MakeRange from mfbt/IntegerRange.h, but baking in the details of AllocKind should allow us to do: for (auto i : AllAllocKinds()) ... for (auto i : ObjectAllocKinds()) ... @@ +2828,5 @@ > + MOZ_ASSERT(backgroundFinalizeState[size_t(thingKind)] == BFS_DONE); > + MOZ_ASSERT(!arenaListsToSweep[size_t(thingKind)]); > + > + arenaListsToSweep[size_t(thingKind)] = arenaLists[size_t(thingKind)].head(); > + arenaLists[size_t(thingKind)].clear(); These size_t() are pretty noisy. Maybe we could replace some of the more obnoxious cases with an inline accessor that takes an AllocKind and returns a reference into the array? This could be pretty simple if we rename arenaLists to arenaLists_ and add a new method ArenaList &arenaLists(AllocKind kind) { return arenaLists_[size_t(kind)]; }. Then we can just swap out the [] for (). This would be fairly trivial if all the AllocKind were named kind instead of the hodgepodge of kind/thingKind that currently exists. At least the compiler will help here. @@ +5218,5 @@ > incrementalSweptArenas.clear(); > > // Join |arenaLists[thingKind]| and |sweepList| into a single list. > ArenaList finalized = sweepList.toArenaList(); > + arenaLists[size_t(thingKind)] = Trailing whitespace.

I guess I was feeling adventurous :) (In reply to Terrence Cole [:terrence] from comment #3) > I think uint8_t is probably the right type for this; we impose this > requirement manually anyway, so we might as well embed it in the type system. Done. > For a separate patch (unless you feel adventurous): It would be nice to > replace ALLOCKIND_LIMIT based iteration with a range-based iteration. > Something like MakeRange from mfbt/IntegerRange.h, but baking in the details > of AllocKind should allow us to do: > > for (auto i : AllAllocKinds()) ... > for (auto i : ObjectAllocKinds()) ... I tried this, and got something that compiled, but I think MSVC's support for range based for is buggy in some way. Things like for (auto i : AllAllocKinds()) MOZ_ASSERT(something involving i); produced a bogus error message about an unexpected '}', and when I got things compiling (by adding braces), jit tests would fail in weird ways. So I think it's desugaring this construct in a weird way that doesn't quite work, and I'm not sure just adding braces to all of these would fix it. It's also possible that I messed up the range somehow (I had to add a new EnumRange class), but as far as I could tell it was correctly looping over all of them. For this patch I added ALL_ALLOC_KINDS() and OBJECT_ALLOC_KINDS() macros instead, so you can do |for (ALL_ALLOC_KINDS(kind))|. I know macros can be footguns, but it's also *much* simpler. If you'd like I can post a range-based patch that applies on top of this (which compiles, but falls over at runtime). > > + arenaLists[size_t(thingKind)].clear(); > > These size_t() are pretty noisy. Maybe we could replace some of the more > obnoxious cases with an inline accessor that takes an AllocKind and returns > a reference into the array? This could be pretty simple if we rename > arenaLists to arenaLists_ and add a new method ArenaList > &arenaLists(AllocKind kind) { return arenaLists_[size_t(kind)]; }. Then we > can just swap out the [] for (). This would be fairly trivial if all the > AllocKind were named kind instead of the hodgepodge of kind/thingKind that > currently exists. At least the compiler will help here. I did something a bit more radical here: I converted all of them to (alias templates of) |mozilla::EnumeratedArray| which can only be indexed with AllocKind, and get (debug-only) runtime range checks from the underlying mozilla::Range class. This lets us keep [] everywhere without casts, and gives us a bit more of the promised enum class type safety :) I think you'll like how this looks (combined with the aforementioned macros), though I guess we might need to check that the complexity gets optimized away (or near as makes no difference). Since these take AllocKind members for their length, I had to add AllocKind::OBJECT_LIMIT and AllocKind::LIMIT. This was a bit awkward since the enum needs to be iterable; hopefully the way I did it is acceptable. I removed ALLOCKIND_LIMIT and ALLOCKIND_OBJECT_LIMIT since they're no longer necessary. > > + arenaLists[size_t(thingKind)] = > > Trailing whitespace. Fixed. Hopefully I didn't introduce any new instances (I need a better check for this).

Oops, that had a bogus include in it. Not sure how it compiled.

Comment on attachment 8576281 [details] [diff] [review] v2.1 - Convert js::gc::AllocKind to an enum class. Review of attachment 8576281 [details] [diff] [review]: ----------------------------------------------------------------- It's a work of art! Brilliant!

Ah, it seems I was using the wrong limits for the iterators - using AllocKind::LIMIT/AllocKind::OBJECT_LIMIT rather than AllocKind::LAST/AllocKind::OBJECT_LAST, jit-tests are now passing! I was sure I eliminated that possibility too, but I guess I messed up somewhere in debugging. In any case, I'll move the range-based for loop stuff to a follow-up since it requires an addition to MFBT.

Carrying forward r=terrence. Try was unhappy because with uint8_t as the underlying type, and AllocKind::OBJECT0 having the value 0, things like MOZ_ASSERT(kind >= AllocKind::OBJECT0) became tautological compares. I removed these comparisons and added a static_assert in case AllocKind::OBJECT0 should ever *not* be 0, as per Terrence's suggestion over IRC. With that change, try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52d9197ad9be

Slight rebase. This bitrots pretty quickly.

There are other GC-related enums, but I don't think they're used as widely as AllocKind. Let's open another bug for them.