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

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript: GC
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8773981 [details] [diff] [review]
1_convert_State_to_enumclass-v0.diff
Attachment #8773981 - Flags: review?(jcoppeard)
(Assignee)

Comment 2

a year ago
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.
Attachment #8773981 - Flags: review?(jcoppeard)
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?
Attachment #8773981 - Flags: review+
(Assignee)

Comment 4

a year ago
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.
(Assignee)

Comment 5

a year ago
Created attachment 8774442 [details] [diff] [review]
1_convert_State_to_enumclass-v1.diff
Attachment #8773981 - Attachment is obsolete: true
Attachment #8774442 - Flags: review+
(Assignee)

Comment 6

a year ago
Created attachment 8774461 [details] [diff] [review]
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.
Attachment #8774461 - Flags: review?(jcoppeard)
(Assignee)

Comment 7

a year ago
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]
2_add_tests-v0.diff

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

Great, thanks for adding this.
Attachment #8774461 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 9

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d35c3a67e91
(Assignee)

Comment 10

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8dbe4594ec3
(Assignee)

Comment 11

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c85767662b63
https://hg.mozilla.org/mozilla-central/rev/2ffdc17e68e0
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.