Closed Bug 1139552 Opened 5 years ago Closed 5 years ago

Turn AllocKind into a C++11 style enum class

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: terrence, Assigned: ehoogeveen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

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.
Attachment #8574290 - Flags: review?(terrence)
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.
Attachment #8574290 - Flags: review?(terrence) → feedback+
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).
Assignee: terrence → emanuel.hoogeveen
Attachment #8574290 - Attachment is obsolete: true
Attachment #8576276 - Flags: review?(terrence)
Oops, that had a bogus include in it. Not sure how it compiled.
Attachment #8576276 - Attachment is obsolete: true
Attachment #8576276 - Flags: review?(terrence)
Attachment #8576281 - Flags: review?(terrence)
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!
Attachment #8576281 - Flags: review?(terrence) → review+
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
Attachment #8576281 - Attachment is obsolete: true
Attachment #8576902 - Flags: review+
Keywords: checkin-needed
Slight rebase. This bitrots pretty quickly.
Attachment #8576902 - Attachment is obsolete: true
Attachment #8577150 - Flags: review+
Blocks: 1143042
https://hg.mozilla.org/mozilla-central/rev/2b9f5019abf1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
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.
Summary: Use C++11 style enums in the GC → Turn AllocKind into a C++11 style enum class
You need to log in before you can comment on or make changes to this bug.