Consider to add nursery purple buffer for main thread

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

This might improve the performance of AddRef/Release calls.
Often used mainthread-only objects would need to use some specific macros to use the buffer.
The idea is that we'd avoid the TLS overhead we currently pay to access the purple buffer, plus we might be able to create a buffer that is guaranteed to not be fragmented, which would improve the locality. We could amortize the cost of accessing the purple buffer by synchronously flushing the purple buffer buffer when it gets full.
I guess it would be good to know how much of the overhead is for TLS compared to dealing with the purple buffer data structure. If that's possible to figure out. I suppose the first step would be to implement a separate AddRef/Release for main thread stuff, and make it go through a global rather than TLS and see how much that helps.
TLS inside libxul requires a function call; how much work that function call does is platform-specific.  On Linux, I think the function call does work on the order of 10-20 instructions, several of which are memory references.
Looks like I started to write a patch. Taking.
Assignee: nobody → bugs
Created attachment 8898891 [details] [diff] [review]
wip

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/ab9127aaa432b7a74b86a884ab46e82b7e804050
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab9127aaa432b7a74b86a884ab46e82b7e804050
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=ab9127aaa432b7a74b86a884ab46e82b7e804050
Comment on attachment 8898891 [details] [diff] [review]
wip

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

::: xpcom/base/nsCycleCollector.cpp
@@ +205,5 @@
> +};
> +
> +#define NURSERY_PURPLE_BUFFER_SIZE 2048
> +bool gNurseryPurpleBufferEnabled = true;
> +NurseryPurpleBufferEntry gNurseryPurpleBufferEntry[NURSERY_PURPLE_BUFFER_SIZE];

Maybe turn the nursery purple buffer into a regular C++ class?

@@ +216,5 @@
> +                                     nsCycleCollectingAutoRefCnt* aRefCnt)
> +{
> +  MOZ_ASSERT(gNurseryPurpleBufferEnabled);
> +  if (gNurseryPurpleBufferEntryCount == NURSERY_PURPLE_BUFFER_SIZE) {
> +    ClearNurseryPurpleBuffer();

Can't this just call SuspectNurseryEntries() directly?

@@ +221,5 @@
> +  }
> +
> +  NurseryPurpleBufferEntry& entry =
> +    gNurseryPurpleBufferEntry[gNurseryPurpleBufferEntryCount];
> +  entry.mPtr = aPtr;

I think you could do entry = {aPtr, aCp, aRefCnt}

@@ +1070,5 @@
>    template<class PurpleVisitor>
>    void VisitEntries(PurpleVisitor& aVisitor)
>    {
> +    Maybe<AutoRestore<bool>> ar;
> +    if (NS_IsMainThread()) {

Maybe add a bool to nsCycleCollector that indicates we're on the main thread so we don't have to do this check all over the place?

@@ +1073,5 @@
> +    Maybe<AutoRestore<bool>> ar;
> +    if (NS_IsMainThread()) {
> +      ar.emplace(gNurseryPurpleBufferEnabled);
> +      gNurseryPurpleBufferEnabled = false;
> +      ClearNurseryPurpleBuffer();

This ends up doing an extra TLS call. Can't this just call SuspectNurseryEntries() directly?

@@ +4095,5 @@
>    SuspectAfterShutdown(aPtr, aCp, aRefCnt, aShouldDelete);
>  }
>  
> +void ClearNurseryPurpleBuffer()
> +{

This should assert that we're on the main thread.

@@ +4106,5 @@
> +bool
> +NS_CycleCollectorSuspectUsingNursery(void* aPtr,
> +                                     nsCycleCollectionParticipant* aCp,
> +                                     nsCycleCollectingAutoRefCnt* aRefCnt)
> +{

This should assert that we're on the main thread.

::: xpcom/base/nsISupportsImpl.h
@@ +226,5 @@
> +    return incrUsingNursery(aOwner, nullptr);
> +  }
> +
> +  MOZ_ALWAYS_INLINE uintptr_t
> +  incrUsingNursery(void* aOwner, nsCycleCollectionParticipant* aCp)

It would be nice if you could template incr and decr over the suspect method. These methods are tricky enough already.
(In reply to Andrew McCreight [:mccr8] from comment #6)
> @@ +216,5 @@
> > +                                     nsCycleCollectingAutoRefCnt* aRefCnt)
> > +{
> > +  MOZ_ASSERT(gNurseryPurpleBufferEnabled);
> > +  if (gNurseryPurpleBufferEntryCount == NURSERY_PURPLE_BUFFER_SIZE) {
> > +    ClearNurseryPurpleBuffer();
> 
> Can't this just call SuspectNurseryEntries() directly?
Nope. SuspectNurseryEntries is a method of CycleCollector, and getting access to that 
requires the tls lookup ClearNurseryPurpleBuffer does.


> @@ +1070,5 @@
> >    template<class PurpleVisitor>
> >    void VisitEntries(PurpleVisitor& aVisitor)
> >    {
> > +    Maybe<AutoRestore<bool>> ar;
> > +    if (NS_IsMainThread()) {
> 
> Maybe add a bool to nsCycleCollector that indicates we're on the main thread
> so we don't have to do this check all over the place?
Perhaps. Though it is checked only in non-hot methods.

> 
> @@ +1073,5 @@
> > +    Maybe<AutoRestore<bool>> ar;
> > +    if (NS_IsMainThread()) {
> > +      ar.emplace(gNurseryPurpleBufferEnabled);
> > +      gNurseryPurpleBufferEnabled = false;
> > +      ClearNurseryPurpleBuffer();
> 
> This ends up doing an extra TLS call. Can't this just call
> SuspectNurseryEntries() directly?
I guess I could pass CycleCollector* as a param and call SuspectNurseryEntries on it.
But this method isn't hot.


> ::: xpcom/base/nsISupportsImpl.h
> @@ +226,5 @@
> > +    return incrUsingNursery(aOwner, nullptr);
> > +  }
> > +
> > +  MOZ_ALWAYS_INLINE uintptr_t
> > +  incrUsingNursery(void* aOwner, nsCycleCollectionParticipant* aCp)
> 
> It would be nice if you could template incr and decr over the suspect
> method. These methods are tricky enough already.
That might work. Would need to change the new suspect method to internally call the old one when needed.
Created attachment 8899288 [details] [diff] [review]
nursery_purple_buffer_2.diff

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/2893c0ecfa01db1f7f2dd977e58c686c345afa46
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=2893c0ecfa01db1f7f2dd977e58c686c345afa46
remote: 
remote: It looks like this try push has talos jobs. Compare performance against a baseline revision:
remote:   https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=2893c0ecfa01db1f7f2dd977e58c686c345afa46
Attachment #8898891 - Attachment is obsolete: true
Attachment #8899288 - Flags: review?(continuation)
Comment on attachment 8899288 [details] [diff] [review]
nursery_purple_buffer_2.diff

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

::: dom/base/FragmentOrElement.cpp
@@ +2143,5 @@
>    // same as nsINode (which nsIContent inherits).
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIContent)
>  NS_INTERFACE_MAP_END
>  
> +NS_IMPL_MAIN_THREAD_ONLY_CYCLE_COLLECTING_ADDREF(FragmentOrElement)

Are you going to do this for RangeItem in a followup?

::: xpcom/base/nsCycleCollector.cpp
@@ +214,5 @@
> +void SuspectUsingNurseryPurpleBuffer(void* aPtr,
> +                                     nsCycleCollectionParticipant* aCp,
> +                                     nsCycleCollectingAutoRefCnt* aRefCnt)
> +{
> +  MOZ_ASSERT(gNurseryPurpleBufferEnabled);

Please add an NS_IsMainThread() assertion to this method.

@@ +223,5 @@
> +  NurseryPurpleBufferEntry& entry =
> +    gNurseryPurpleBufferEntry[gNurseryPurpleBufferEntryCount];
> +  entry.mPtr = aPtr;
> +  entry.mParticipant = aCp;
> +  entry.mRefCnt = aRefCnt;

Just do |entry = {aPtr, aCp, aRefCnt}| here. I think it is easier to read.

@@ +3533,5 @@
>  void
> +nsCycleCollector::SuspectNurseryEntries()
> +{
> +  MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!");
> +  CheckThreadSafety();

You don't need the CheckThreadSafety() call because you have the other assert.
Attachment #8899288 - Flags: review?(continuation) → review+
Yeah, RangeItem would be a followup.  It is in general rarely used object after all.
Created attachment 8899582 [details] [diff] [review]
nursery_purple_buffer_3.diff

Comment 12

a year ago
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f63a7cba6c08
add a nursery for purple buffer to allow faster addref/release on the main thread, r=mccr8
https://hg.mozilla.org/mozilla-central/rev/f63a7cba6c08
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.