1391423 - Consider to add nursery purple buffer for main thread

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Looks like I started to write a patch. Taking.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Yeah, RangeItem would be a followup.  It is in general rarely used object after all.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: nursery_purple_buffer_3.diff

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f63a7cba6c08