1467842 - Don't take the exclusive access lock during GC

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

We don't allow collection of the atoms zone while any off-thread parsing is happening so we don't need to take the exclusive access lock during GC.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1467842-unlock-trace-session

This patch replaces use of AutoLockForExclusiveAccess to access the atoms zone with AutoAccessAtomsZone.  The latter can be constructed from the former or from  AutoCheckCanAccessAtomsDuringGC, which asserts that we are inside GC and no off-thread parsing is happening.

AutoTraceSession then only takes the exclusive access lock if it's being used for heap tracing (i.e. not GC).

This is in preparation for splitting the atom table which will remove use of the exclusive access lock.

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8984514 [details] [diff] [review]
bug1467842-unlock-trace-session

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

This is cool!

::: js/src/gc/GCInternals.h
@@ +56,5 @@
>      explicit AutoTraceSession(JSRuntime* rt, JS::HeapState state = JS::HeapState::Tracing);
>      ~AutoTraceSession();
>  
> +    // Constructing an AutoTraceSession takes the exclusive access lock unless
> +    // it is being used for GC.

This comment is confusing; what does "it" refer to? AutoTraceSession or the exclusive access lock? I think you're saying

  // Constructing an AutoTraceSession takes the exclusive access lock for non-GC uses.

or perhaps

  // AutoTraceSession will need to take the exclusive access lock when this is not a GC session.

or... something?

@@ +98,3 @@
>  
> +    AutoFinishGC finishGC;
> +    AutoTraceSession session;

Hm. You prefer this to having AutoPrepareForTracing inherit from AutoTraceSession, and calling FinishGC() in the constructor? That would make the name sound a little funny, I guess, but what if this were AutoExclusiveTraceSession (or AutoNonGCTraceSession, but maybe that sounds awkward)? Then instead of

    PrepareForTracing prep(cx);
    doSomething(prep.session);

it would just be

    AutoExclusiveTraceSession session(cx);
    doSomething(session);

::: js/src/vm/JSContext.h
@@ +543,5 @@
>      // with AutoDisableCompactingGC which uses this counter.
>      js::ThreadData<unsigned> compactingDisabledCount;
>  
>      bool canCollectAtoms() const {
> +        // TODO: Can we improve this by collecting if !isOffThreadParseRunning()?

Ooh, that's interesting. File an "Investigate..." bug and put the bug number here.

@@ +1187,5 @@
> +//  - the current thread holds the exclusive access lock (off-thread parsing may
> +//    be running and this must also take the lock for access)
> +//
> +//  - the GC is running and is collecting the atoms zone (this cannot be started
> +//    while off-thread parsing is happening)

Nice comment

::: js/src/vm/Runtime.h
@@ +703,3 @@
>  
>      // Set of all live symbols produced by Symbol.for(). All such symbols are
> +    // allocated in the atoms zone. Reading or writing symbol registry requires

lost a "the"

@@ +894,5 @@
> +    bool isOffThreadParsingBlocked() const {
> +        return offThreadParsingBlocked_;
> +    }
> +    void setOffThreadParsingBlocked(bool blocked) {
> +        MOZ_ASSERT(offThreadParsingBlocked_ != blocked);

MOZ_ASSERT(!isOffThreadParseRunning())?

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #2)
> This comment is confusing; what does "it" refer to? AutoTraceSession or the
> exclusive access lock?

Oh right, yes it means the trace session.  Fixed.

> Hm. You prefer this to having AutoPrepareForTracing inherit from
> AutoTraceSession, and calling FinishGC() in the constructor? 

The idea is to do the FinishGC() before construction of the trace session.  I like the idea of having two separate classes for GC/tracing though, I think that's better that what I've got here.

> Ooh, that's interesting. File an "Investigate..." bug and put the bug number
> here.

Filed bug 1468422.  Although it sounds harder now I try to describe how to do it.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1467842-refactor-trace-session

Patch to refactor these RAII clases into:
 - an abstract base class, AutoHeapSession
 - a class used by GC, AutoGCSession which has a Maybe<AutoCheckCanAccessAtomsDuringGC>
 - a class used for tracing, AutoTraceSession that inherits from AutoLockForExclusiveAccess

Comment 5

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8985372 [details] [diff] [review]
bug1467842-refactor-trace-session

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

I like it. Splitting up the type of trace session makes it more clear in the function signatures.

::: js/src/gc/GCInternals.h
@@ +103,4 @@
>  
> +// This class should be used by any code that needs to exclusive access to the
> +// heap in order to trace through it.
> +class MOZ_RAII AutoPrepareForTracing : public AutoFinishGC,

Is the AutoFinishGC just a way to sneak in a FinishGC() call before constructing the AutoTraceSession, or are there external users that need to know an AutoPrepareForTracing isa AutoFinishGC? If not, then it might be a little more clear to use private inheritance rather than public.

Comment 6

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #5)
> Is the AutoFinishGC just a way to sneak in a FinishGC() call before
> constructing the AutoTraceSession, or are there external users that need to
> know an AutoPrepareForTracing isa AutoFinishGC? If not, then it might be a
> little more clear to use private inheritance rather than public.

Yes, it's the former.  I'll do that.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b05310b2c7
Don't take the exclusive access lock during GC r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f50305f72ab
Refactor heap state RAII classes r=sfink

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/84b05310b2c7
https://hg.mozilla.org/mozilla-central/rev/4f50305f72ab