Closed Bug 1147670 Opened 5 years ago Closed 5 years ago

Remove duplicate IsMarked/IsAboutToBeFinalized for off-thread usage


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox40 --- fixed


(Reporter: terrence, Assigned: terrence)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

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

Erk, I forgot about cross compartment keys. I guess we may need to check the runtime flag instead.
Attachment #8583470 - Flags: review?(jcoppeard)
Seems to work locally checking the runtime state.
Attachment #8583470 - Attachment is obsolete: true
Attachment #8583489 - Flags: review?(jcoppeard)
Comment on attachment 8583489 [details] [diff] [review]

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.
Attachment #8583489 - Flags: review?(jcoppeard) → review+
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.
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.

(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.
Blocks: 1148534
Backed out in because something in this push caused frequent 10.10 debug devtools-2 assertion failures like
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.