Closed Bug 1515648 Opened Last year Closed 11 months ago

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

Categories

(Core :: JavaScript: GC, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(5 files, 7 obsolete files)

1.68 KB, patch
jonco
: review+
Details | Diff | Splinter Review
1.67 KB, patch
jonco
: review+
Details | Diff | Splinter Review
1.38 KB, patch
jonco
: review+
Details | Diff | Splinter Review
8.12 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
1.09 KB, patch
allstars.chh
: review+
Details | Diff | Splinter Review
Attached patch Part 4: fix in SavedStacks.cpp (obsolete) — Splinter Review
Attachment #9032698 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #9032911 - Flags: review?(jcoppeard)
Attachment #9032913 - Flags: review?(jcoppeard)
Attachment #9032916 - Flags: review?(jcoppeard)
Attachment #9032911 - Flags: review?(jcoppeard) → review+
Attachment #9032918 - Flags: review?(jcoppeard)
Attachment #9032919 - Flags: review?(jcoppeard)
Attachment #9032913 - Flags: review?(jcoppeard) → review+
Attachment #9032919 - Flags: review?(jcoppeard) → review+
Summary: Assert barriers don't fire during collection and fix the places this happens → Assert read barriers don't fire during collection and fix the places this happens
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.
Attachment #9032916 - Flags: review?(jcoppeard) → review+
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.
Attachment #9032918 - Flags: review?(jcoppeard) → review+
Pushed by allstars.chh@gmail.com:
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
Depends on: 1517158
You need to log in before you can comment on or make changes to this bug.