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

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years 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

2 years 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

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

Comment 2

2 years 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

2 years 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

2 years 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

2 years 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

2 years 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+

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c85767662b63
https://hg.mozilla.org/mozilla-central/rev/2ffdc17e68e0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years 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.