1334927 - Handle multiple contexts per runtime in the SPS profiler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off! Don't needinfo me!]]

Attachment: patch

SPS assumes in a number of places that the runtime has only one main thread.  Fixing this to tolerate multiple threads in a cooperatively threaded runtime doesn't seem too tricky (this is just the JS side of things, though; the profiler will of course need a fair amount of work when the browser itself is multithreaded).  Preemptive multithreading will need more (probably substantially more) work to handle.

Comment 1

[Brian Hackett [Laid off! Don't needinfo me!]]

Attachment: patch

This patch adds a AutoProhibitActiveContextChange class which makes things tidier and better separated here and will be useful in later patches for bug 1323066.

Comment 2

[Shu-yu Guo [:shu]]

Comment on attachment 8831672 [details] [diff] [review]
patch

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

::: js/src/jit/CompileWrappers.cpp
@@ -93,5 @@
>  
>  const void*
>  CompileRuntime::addressOfActiveJSContext()
>  {
> -    return &runtime()->activeContext;

Where's the patch that added activeContext? I don't see it in my tree.

Comment 3

[Brian Hackett [Laid off! Don't needinfo me!]]

(In reply to Shu-yu Guo [:shu] from comment #2)
> Comment on attachment 8831672 [details] [diff] [review]
> patch
> 
> Review of attachment 8831672 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/CompileWrappers.cpp
> @@ -93,5 @@
> >  
> >  const void*
> >  CompileRuntime::addressOfActiveJSContext()
> >  {
> > -    return &runtime()->activeContext;
> 
> Where's the patch that added activeContext? I don't see it in my tree.

Bug 1334837 will add JSRuntime::activeContext.

Comment 4

[Shu-yu Guo [:shu]]

Comment on attachment 8831672 [details] [diff] [review]
patch

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

This seems fine to me, but I'm lacking the big picture for how this is intended to be used.

::: js/src/vm/Runtime.h
@@ +304,5 @@
> +    mozilla::Atomic<JSContext*, mozilla::ReleaseAcquire> activeContext_;
> +
> +    // All contexts participating in cooperative scheduling. All threads other
> +    // than |activeContext_| are suspended.
> +    js::ActiveThreadData<js::Vector<JSContext*, 4, js::SystemAllocPolicy>> cooperatingContexts_;

I couldn't find ActiveThreadData either. I tried looking for it in one of the other bugs and only found ProtectedData and its subclasses, with ActiveThread not among them. I imagine it's for DEBUG-only checks that ref() is being called from the active thread? That is, the data is always single writer single reader?

@@ +307,5 @@
> +    // than |activeContext_| are suspended.
> +    js::ActiveThreadData<js::Vector<JSContext*, 4, js::SystemAllocPolicy>> cooperatingContexts_;
> +
> +    // Count of AutoProhibitActiveContextChange instances on the active context.
> +    js::ActiveThreadData<size_t> activeContextChangeProhibited;

Should have a trailing _ by convention for private fields.

@@ +329,5 @@
> +        return false;
> +    }
> +#endif
> +
> +    class MOZ_RAII AutoProhibitActiveContextChange

AFAICT this class and |activeContextChangeProhibited| field is used for DEBUG-only checks. Is the a reason this class and the field are not DEBUG-only a subsequent patch?

The name is a bit confusing, as this doesn't actually prohibit changing the active context. It's more like AutoAssertNoActiveContextChange right now. I don't know future plans though.

Comment 5

[Brian Hackett [Laid off! Don't needinfo me!]]

(In reply to Shu-yu Guo [:shu] from comment #4)
> ::: js/src/vm/Runtime.h
> @@ +304,5 @@
> > +    mozilla::Atomic<JSContext*, mozilla::ReleaseAcquire> activeContext_;
> > +
> > +    // All contexts participating in cooperative scheduling. All threads other
> > +    // than |activeContext_| are suspended.
> > +    js::ActiveThreadData<js::Vector<JSContext*, 4, js::SystemAllocPolicy>> cooperatingContexts_;
> 
> I couldn't find ActiveThreadData either. I tried looking for it in one of
> the other bugs and only found ProtectedData and its subclasses, with
> ActiveThread not among them. I imagine it's for DEBUG-only checks that ref()
> is being called from the active thread? That is, the data is always single
> writer single reader?

ActiveThreadData is also added in bug 1334837.  Yes, it ensures in debug builds that accesses only occur on the currently active cooperative thread.

> @@ +329,5 @@
> > +        return false;
> > +    }
> > +#endif
> > +
> > +    class MOZ_RAII AutoProhibitActiveContextChange
> 
> AFAICT this class and |activeContextChangeProhibited| field is used for
> DEBUG-only checks. Is the a reason this class and the field are not
> DEBUG-only a subsequent patch?
> 
> The name is a bit confusing, as this doesn't actually prohibit changing the
> active context. It's more like AutoAssertNoActiveContextChange right now. I
> don't know future plans though.

Hmm, I can change the check to a MOZ_RELEASE_ASSERT, I think it'd be good to make absolutely sure we don't get unexpected thread switches.  None of the places this class is used should be hot.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett (:bhackett) from comment #5)
> (In reply to Shu-yu Guo [:shu] from comment #4)
> > ::: js/src/vm/Runtime.h
> > @@ +329,5 @@
> > > +        return false;
> > > +    }
> > > +#endif
> > > +
> > > +    class MOZ_RAII AutoProhibitActiveContextChange
> > 
> > AFAICT this class and |activeContextChangeProhibited| field is used for
> > DEBUG-only checks. Is the a reason this class and the field are not
> > DEBUG-only a subsequent patch?
> > 
> > The name is a bit confusing, as this doesn't actually prohibit changing the
> > active context. It's more like AutoAssertNoActiveContextChange right now. I
> > don't know future plans though.
> 
> Hmm, I can change the check to a MOZ_RELEASE_ASSERT, I think it'd be good to
> make absolutely sure we don't get unexpected thread switches.  None of the
> places this class is used should be hot.

Yeah, let's do MOZ_RELEASE_ASSERT.

Comment 7

[Brian Hackett [Laid off! Don't needinfo me!]]

Attachment: followup

This followup fixes a place that turned up during testing where the runtime is used to get the context of a thread whose profiling frames are being iterated.  Since ProfilingFrameIterator was originally passed the context to iterate over, fixing this just requires keeping track of that context more closely.

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8833492 [details] [diff] [review]
followup

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

::: js/src/vm/Stack.cpp
@@ +1940,5 @@
>      }
>  
>      // Extract the stack for the entry.  Assume maximum inlining depth is <64
>      const char* labels[64];
> +    uint32_t depth = entry.callStackAtAddr(cx_->runtime(), jitIter().returnAddressToFp(), labels, 64);

While you're here, could you switch 64 to ArrayLength(labels)?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aac89d4610d
Handle multiple contexts per runtime in the Gecko profiler, r=shu.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1aac89d4610d