1288793 - Make State a typed enum class, macroize expansions, and test formatter output

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Bug 1288566 should not have happened.

We could have prevented it by:

1) Taking more aggressive advantage of C++11: an enum class would have made it more convenient to use a switch and thus complained about the missing case.
2) Taking better advantage of C: using a macro to expand the list, as we do elsewhere would have prevented us from dropping the entry.
3) Having the StateName function inline next to the enum: would have made it much harder to miss when updating the enum.
4) Having any sort of tests at all on this functionality. The JSON export was only for MemChaser, so we didn't think it necessary. That was dumb.

So I'd like to do all of those in this bug.

Comment 1

[Terrence Cole [:terrence]]

Attachment: 1_convert_State_to_enumclass-v0.diff

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8773981 [details] [diff] [review]
1_convert_State_to_enumclass-v0.diff

Review of attachment 8773981 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TestingFunctions.cpp
@@ -877,5 @@
> -        state = "compact";
> -    else if (globalState == gc::DECOMMIT)
> -        state = "decommit";
> -    else
> -        MOZ_CRASH("Unobserveable global GC state");

I'm not sure why these need to be different from the output of StateName, so I just fixed the tests to use those names.

::: js/src/jit-test/tests/gc/incremental-abort.js
@@ +49,5 @@
>  testAbort(10, 10000, 10000);
> +testAbort(10, 10000, 10000, "Mark");
> +testAbort(10, 10000, 10000, "Sweep");
> +testAbort(10, 10000, 10000, "Compact");
> +testAbort(10, 10000, 10000, "Decommit");

We were missing tests for aborting during decommit. Whoops!

::: js/src/jsgc.cpp
@@ -5710,5 @@
>          break;
>        }
> -
> -      default:
> -        MOZ_CRASH("Invalid incremental GC state");

I verified that leaving out cases emits a compiler warning, so I don't think these are necessary anymore, given our warn-as-error build.

@@ +7604,5 @@
> +      case State::Sweep: return "Sweep";
> +      case State::Finalize: return "Finalize";
> +      case State::Compact: return "Compact";
> +      case State::Decommit: return "Decommit";
> +    }

I'm a bit torn on whether we should macro expand these or not. This would be the only user and macro expansion does come with some cognitive overhead. Moreover, we warn when adding new states and at this point, I don't really see us adding new states for a good long while.

On the other hand, the code here does have a bug in it. I'd better get a new patch posted using macro expansion, because I kinda just proved myself wrong. :-P

::: js/src/jsgc.h
@@ +43,5 @@
>  
>  struct FinalizePhase;
>  
> +enum class State {
> +    NotActive,

I've always thought that NO_INCREMENTAL was a terrible name. It took me forever to grok that it doesn't actually have much of anything to do with IGC at all and does not in any way imply that the GC is non-incremental.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8773981 [details] [diff] [review]
1_convert_State_to_enumclass-v0.diff

Review of attachment 8773981 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: js/src/jsgc.cpp
@@ +7597,5 @@
>  const char*
>  StateName(State state)
>  {
> +    switch(state) {
> +      case State::NotActive: return "None";

For consistency I think we should call this state "NotActive".

Oh, that was the bug, right?

Comment 4

[Terrence Cole [:terrence]]

Indeed! I added an abort test for Decommit while I was updating the names (as I noticed the missing name in the list). That's failing horribly. I'll file a followup.

Comment 5

[Terrence Cole [:terrence]]

Attachment: 1_convert_State_to_enumclass-v1.diff

Comment 6

[Terrence Cole [:terrence]]

Attachment: 2_add_tests-v0.diff

I had trouble getting an actual incremental GC to happen with no garbage on the heap. It seems we can request a slice message even in a non-incremental GC, so we should still be testing everything we want to test here.

Comment 7

[Terrence Cole [:terrence]]

Looks like the reason the abort tests are failing is that we don't yield if there is no work to do, and the test was not large enough to trigger any decommit. I've made a note about it (and similarly Finalize).

Comment 8

[Jon Coppeard (:jonco)]

Comment on attachment 8774461 [details] [diff] [review]
2_add_tests-v0.diff

Review of attachment 8774461 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks for adding this.

Comment 9

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d35c3a67e91

Comment 10

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8dbe4594ec3

Comment 11

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c85767662b633604936c5b5e4869770423f5ec01
Bug 1288793 - Part 1: Convert js::gc::State to an enum class; r=jonco

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffdc17e68e06e837dc9d8cea0741e3f6bda8631
Bug 1288793 - Part 2: Test slice callback and message formatting; r=jonco

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c85767662b63
https://hg.mozilla.org/mozilla-central/rev/2ffdc17e68e0