Closed Bug 1288793 Opened 5 years ago Closed 5 years ago

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


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox50 --- fixed


(Reporter: terrence, Assigned: terrence)


(Blocks 1 open bug)



(2 files, 1 obsolete file)

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.
Attachment #8773981 - Flags: review?(jcoppeard)
Comment on attachment 8773981 [details] [diff] [review]

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.
Attachment #8773981 - Flags: review?(jcoppeard)
Comment on attachment 8773981 [details] [diff] [review]

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


::: 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?
Attachment #8773981 - Flags: review+
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.
Attachment #8773981 - Attachment is obsolete: true
Attachment #8774442 - Flags: review+
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.
Attachment #8774461 - Flags: review?(jcoppeard)
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 on attachment 8774461 [details] [diff] [review]

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

Great, thanks for adding this.
Attachment #8774461 - Flags: review?(jcoppeard) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.