1147670 - Remove duplicate IsMarked/IsAboutToBeFinalized for off-thread usage

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 4.6.1_simplify_ismarkedfromanythread-v0.diff

Instead of having a second call, let's just assert that we're either on the main thread or are sweeping in the thing's zone. This is almost as strong a guarantee and results in a ton less, and less ugly, code.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 8583470 [details] [diff] [review]
4.6.1_simplify_ismarkedfromanythread-v0.diff

Erk, I forgot about cross compartment keys. I guess we may need to check the runtime flag instead.

Comment 2

[Terrence Cole [:terrence]]

Attachment: 4.6.1_fixup_ismarkedfromanythread-v1.diff

Seems to work locally checking the runtime state.

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

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8583489 [details] [diff] [review]
4.6.1_fixup_ismarkedfromanythread-v1.diff

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

This looks good to me.

The try build is busted but it doesn't seem to be related to this change from a first glance - something about marking symbols from workers is causing us to assert.  I think we might need a specialization of DoMarking() for symbols that checks isWellKnownSymbol().

::: js/src/gc/Marking.cpp
@@ +795,5 @@
>  IsMarked(T **thingp)
>  {
>      MOZ_ASSERT_IF(!ThingIsPermanentAtom(*thingp),
> +                  CurrentThreadCanAccessRuntime((*thingp)->runtimeFromAnyThread()) ||
> +                  (*thingp)->runtimeFromAnyThread()->gc.state() == SWEEP);

For the sweeping condition, can we check isHeapCollecting() too?  The state will be SWEEP in between incremental slices too.

@@ +819,5 @@
>  IsAboutToBeFinalized(T **thingp)
>  {
>      MOZ_ASSERT_IF(!ThingIsPermanentAtom(*thingp),
> +                  CurrentThreadCanAccessRuntime((*thingp)->runtimeFromAnyThread()) ||
> +                  (*thingp)->runtimeFromAnyThread()->gc.state() == SWEEP);

Ditto above and maybe factor this check out too.

Comment 4

[Terrence Cole [:terrence]]

Gah! Try build is busted because of this unexpected definition:
  template <> bool ThingIsPermanentAtom<JS::Symbol>(JS::Symbol *sym) { return sym->isWellKnownSymbol(); }

It looks like we just make symbols look like permanent atoms for the sake of this check, which is why I didn't catch it. I even remember being a bit confused as to why we didn't need a well-known-symbol check to match the permanent-atom check. :-(

I'll add another overload for DoMarking<Symbol> and rip out ThingIsPermanentAtom entirely.

Comment 5

[Terrence Cole [:terrence]]

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

Comment 6

[Terrence Cole [:terrence]]

And there was some bustage from MSVC being dumb about array[1] not automatically upcasting to BarriedBase<T>* like array* does. I've added begin()/end() methods to the containers that failed to compile there.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1216bd1e667e


(In reply to Terrence Cole [:terrence] from comment #4)
> I'll add another overload for DoMarking<Symbol> and rip out
> ThingIsPermanentAtom entirely.

This check is actually handy to have in this form, so I've renamed it to ThingIsPermanentAtomOrWellKnownSymbol and refectored DoMarking so that it is only the precondition checks that are split out. Seems a bit clearer that way.

Comment 7

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1216bd1e667e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a9d166af96b

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd8c4ec41fa

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/99415fbccf83 because something in this push caused frequent 10.10 debug devtools-2 assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8197104&repo=mozilla-inbound

Comment 9

[Terrence Cole [:terrence]]

Fixed the assertion in the profiler and it seems to be fine now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb58f09f6311
https://hg.mozilla.org/integration/mozilla-inbound/rev/1012996e7a49

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1012996e7a49