1515648 - Assert read barriers don't fire during collection and fix the places this happens

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Fix the TODO in https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/js/src/gc/Cell.h#355

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: WIP Part 1.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: WIP Part 2.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: WIP Part 3.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: WIP Part 4.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: WIP Part 5.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: Part 1: Assert read barriers won't fire during collection.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: Part 2: fix in Shape.cpp

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: Part 3: use unbarrieredGet() for Debugger

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: Part 4: fix in SavedStacks.cpp

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: Part 5: fix in EnvironmentObject.cpp

Comment 11

[Jon Coppeard (:jonco)]

Comment on attachment 9032916 [details] [diff] [review]
Part 3: use unbarrieredGet() for Debugger

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

I'm slightly wary about the places that are called both from GC and from outside it, but I think this is fine.

In general we don't need a comment to use unbarrieredGet() in a method that's only called during GC, but we should add assertions that this is the case.

::: js/src/gc/Zone.cpp
@@ +352,5 @@
>  
>      for (GlobalObject::DebuggerVector::Range r = dbgs->all(); !r.empty();
>           r.popFront()) {
> +      // Use unbarrieredGet() to prevent triggering read barrier while collecting.
> +      if (!r.front().unbarrieredGet()->debuggeeIsBeingCollected(rt->gc.majorGCCount())) {

This doesn't need a comment, but can you add an assertion at the top of this method that this it is only called during GC?

::: js/src/vm/Debugger.cpp
@@ +3036,5 @@
>  /* static */ bool Debugger::isObservedByDebuggerTrackingAllocations(
>      const GlobalObject& debuggee) {
>    if (auto* v = debuggee.getDebuggers()) {
>      for (auto p = v->begin(); p != v->end(); p++) {
> +      // Use unbarrieredGet() to prevent triggering read barrier while collecting.

Can you add something like "this is safe as long as dbg does not escape" to the comment?

@@ +3194,5 @@
>        const GlobalObject::DebuggerVector* debuggers = global->getDebuggers();
>        MOZ_ASSERT(debuggers);
>        for (auto p = debuggers->begin(); p != debuggers->end(); p++) {
> +        // Use unbarrieredGet() to prevent triggering read barrier while collecting.
> +        Debugger* dbg = p->unbarrieredGet();

Don't need this comment, but please add assertion that we're in GC at the top.

@@ +4205,5 @@
>    MOZ_ASSERT(p != vec->end());
>    return p;
>  }
>  
> +// a ReadBarriered version for findDebuggerInVector

nit: The coding style says comments should be full sentences, so this needs to start with a captial letter and end with a full stop.

I tried to work out why this was necessary but I couldn't see it and filed bug 1515934.

Can you add a comment here referencing this bug?

::: js/src/vm/Realm.cpp
@@ +795,5 @@
>            ? unsafeUnbarrieredMaybeGlobal()
>            : maybeGlobal();
>    const GlobalObject::DebuggerVector* v = global->getDebuggers();
>    for (auto p = v->begin(); p != v->end(); p++) {
> +    // Use unbarrieredGet() to prevent triggering read barrier while collecting.

Please add "this is safe as long as dbg does not escape".

::: js/src/vm/SavedStacks.cpp
@@ +1694,4 @@
>      // The set of debuggers had better not change while we're iterating,
>      // such that the vector gets reallocated.
>      MOZ_ASSERT(dbgs->begin() == begin);
> +    // Use unbarrieredGet() to prevent triggering read barrier while collecting.

Ditto above, please add comment about dbgp not escaping.

Comment 12

[Jon Coppeard (:jonco)]

Comment on attachment 9032918 [details] [diff] [review]
Part 4: fix in SavedStacks.cpp

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

::: js/src/vm/SavedStacks.cpp
@@ +1675,5 @@
>    return true;
>  }
>  
>  void SavedStacks::chooseSamplingProbability(Realm* realm) {
> +  // Use unbarriered version to prevent triggering read barrier while collecting.

As previous comments, please add comment about global not escaping.

Comment 13

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: Part 3: use unbarrieredGet() for Debugger. v2

addressed comment 11.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Attachment: Part 4: fix in SavedStacks.cpp. v2

addressed comment 12.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d706269aea5
Part 1: Assert read barriers won't fire during collection. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe391c74c65
Part 2: fix in Shape.cpp. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c07de1d4f4
Part 3: use unbarrieredGet() for Debugger. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/64e9482f70bc
Part 4: fix in SavedStacks.cpp. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/16823b6a72c5
Part 5: fix in EnvironmentObject.cpp. r=jonco

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/3d706269aea5
https://hg.mozilla.org/mozilla-central/rev/8fe391c74c65
https://hg.mozilla.org/mozilla-central/rev/c4c07de1d4f4
https://hg.mozilla.org/mozilla-central/rev/64e9482f70bc
https://hg.mozilla.org/mozilla-central/rev/16823b6a72c5