Closed Bug 1334927 Opened 3 years ago Closed 3 years ago

Handle multiple contexts per runtime in the SPS profiler

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(2 files, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8831568 - Flags: review?(shu)
Attached patch patchSplinter Review
This patch adds a AutoProhibitActiveContextChange class which makes things tidier and better separated here and will be useful in later patches for bug 1323066.
Attachment #8831568 - Attachment is obsolete: true
Attachment #8831568 - Flags: review?(shu)
Attachment #8831672 - Flags: review?(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.
(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 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.
Attachment #8831672 - Flags: review?(shu) → review+
(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.
(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.
Attached patch followupSplinter Review
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.
Attachment #8833492 - Flags: review?(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)?
Attachment #8833492 - Flags: review?(shu) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1aac89d4610d
Handle multiple contexts per runtime in the Gecko profiler, r=shu.
https://hg.mozilla.org/mozilla-central/rev/1aac89d4610d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.