789919 - (snow-white) Investigate if refcnt of cycle collectable objects could be size_t-2 so that 2 bits could be used for flagging whether the object is in purple buffer and whether it is actually purple

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

I was thinking an approach to not store purple buffer entry at all in objects.
They would just store information whether they have been added to purple buffer.
This way we could save indirect addref/release which happens currently when
refcount is actually stored in the pbe.
Adding something to pb might become too slow, since purple buffer would have be a hashtable.

Comment 1

[Andrew McCreight [:mccr8]]

Another approach to avoiding that indirection is to have a stack of things you've put into the purple buffer.  When you go to add something to the purple buffer, you just shove it onto the end of some array. The hope is that because all purple things are being put onto a single array then it is more likely to be in the cache than some random object. This is similar to what the concurrent CC work does, though I don't think they do it for speed reasons in particular.

Comment 2

[Andrew McCreight [:mccr8]]

You can either steal a bit from the ref count to indicate whether something is purple or not, or maybe leave the ref count untagged and instead tag the address you push onto the purple stack to indicate that it is unpurpling. Then you go through it in order and cancel things out as appropriate.

Comment 3

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

How could I remove the tag from the address which has been put to the stack?
Adding anything to the stack/hashtable/array is slow, and address of the object should be
there only once, and removed if the object is deleted.

Comment 4

[Andrew McCreight [:mccr8]]

Setting aside the speed issue for the moment, what you do is, every time you suspect an object, you push its pointer onto the purple stack. Every time you forget an object, you push its pointer onto the purple stack, with the low bit set.

An object is actually in the purple buffer if the last time it appears in the stack, it does not have its low bit set. To build the actual set of purple buffer roots, you read from the end. If you see an object with its low bit set, you add it to a hash set of seen objects. If you see an object without its low bit set, you add it to the graph if it isn't in the set of seen objects, then add it to the set of seen objects.

Comment 5

[Andrew McCreight [:mccr8]]

In terms of performance, if you use a purple bit, then you have additional branches and various bit twiddling operations. If you don't use a purple bit, then you touch the stack more often, and the stack will get bigger. If the purple stack is hot enough, then maybe it will end up being in the cache all the time, and the push cost won't be too bad.

Comment 6

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

ok, I understood you correctly, and I think it is slow, or at least memory hungry :)

Comment 7

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

Attachment: v1

Doesn't seem to leak in the simple cases, and doesn't crash either.
And speeds up various Addref/Release heavy tests up to 7%.

https://tbpl.mozilla.org/?tree=Try&rev=c788380aeefd

Comment 8

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

Attachment: v1.1

Maybe this compiles on 64bit debug osx
https://tbpl.mozilla.org/?tree=Try&rev=bbf1b42b4a4a

Comment 9

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

ok, "tiny" bit crashy on try :/

Comment 10

[Andrew McCreight [:mccr8]]

I haven't looked at your crashes, but take a look at my patch in bug 850065 for where I call NS_CycleCollectorObjectDying.  There are a few classes, particularly in XPConnect, that are ref counted and cycle collected that don't use the ref count macros.  Those caused me a fair bit of grief.  So you'll have to fix up those AddRef/Release functions manually.

Comment 11

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

Attachment: v1.3

https://tbpl.mozilla.org/?tree=Try&rev=05cafa4e4115

Comment 12

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

Attachment: v1.4

https://tbpl.mozilla.org/?tree=Try&rev=9279d8711b0d

Better to kill snow white at the right time.

Comment 13

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

Attachment: v1.5

https://tbpl.mozilla.org/?tree=Try&rev=84b3a7f4c4b1

1.4 seemed to pass tests on linux-debug, so pushing extend testing to 
other platforms, and fix a nit.

Comment 14

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

Attachment: v1.6

Try to kill snow white more often
https://tbpl.mozilla.org/?tree=Try&rev=5563557e6d4f

Comment 15

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

Attachment: v1.8

https://tbpl.mozilla.org/?tree=Try&rev=65fc0b898c56

Comment 16

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

Attachment: v1.13

https://tbpl.mozilla.org/?tree=Try&rev=d9ef17155c83

I think this might now pass the tests. IDB is indeed buggy and the tests
don't pass if dtor doesn't run at certain time.
As a temporary fix, LastRelease is used to keep the old behavior.

Andrew, you may want to start to look at this.

Comment 17

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

(That is a kind of feedback/review? )

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 763163 [details] [diff] [review]
v1.13

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

Here are some comments.  More tomorrow, hopefully.

::: rdf/base/src/nsInMemoryDataSource.cpp
@@ +768,5 @@
>  
>      mForwardArcs.ops = nullptr;
>      mReverseArcs.ops = nullptr;
>      mPropagateChanges = true;
> +    MOZ_COUNT_CTOR(InMemoryDataSource);

What's this about?

::: xpcom/base/nsCycleCollector.cpp
@@ +157,5 @@
>  
>  #define DEFAULT_SHUTDOWN_COLLECTIONS 5
>  
>  // One to do the freeing, then another to detect there is no more work to do.
> +#define NORMAL_SHUTDOWN_COLLECTIONS 3

Why is this 3 now?

::: xpcom/glue/nsISupportsImpl.h
@@ +111,5 @@
>  
> +#define NS_IN_PURPLE_BUFFER (1 << 0)
> +#define NS_IS_PURPLE (1 << 1)
> +#define NS_REFCOUNT_CHANGE (1 << 2)
> +#define NS_REFCOUNT_VALUE(_val) (_val >> 2)

Instead of this macro, can you make this the definition of get(), and call get() wherever else you are using NS_REFCOUNT_VALUE?

@@ +122,3 @@
>    {}
>  
> +  nsCycleCollectingAutoRefCnt(uintptr_t aValue)

Weird.  It doesn't look like this variant is used anywhere, so maybe you can delete it?

@@ +159,3 @@
>      }
> +    mRefCntAndFlags -= NS_REFCOUNT_CHANGE;
> +    mRefCntAndFlags |= (NS_IN_PURPLE_BUFFER | NS_IS_PURPLE);

This line and the one before it are the same in both cases. Can you save the purpleness in a local variable, set the flags, then check the purpleness and call Suspect3 if it is set?  Would that make better code here?  Also, does Suspect3 change the value of mRefCntAndFlags? If not, it seems like both cases can just do the same return.

@@ +276,5 @@
>  {                                                                             \
>    NS_IMPL_CC_NATIVE_ADDREF_BODY(_class)                                       \
>  }
>  
> +#define NS_IMPL_CYCLE_COLLECTING_NATIVE_RELEASE_WITH_LAST_RELEASE(_class, _last) \

When does a class need to use this version?

Comment 19

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

Attachment: v1.14

small changes

Comment 20

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

Comment on attachment 763286 [details] [diff] [review]
v1.14

There is probably still one issue lurking somewhere in devtools.

Comment 21

[Andrew McCreight [:mccr8]]

Comment on attachment 763286 [details] [diff] [review]
v1.14

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

Here are some more comments.  I'll look at it some more tomorrow, there are still a bunch of details I'm not sure of.

::: xpcom/base/nsCycleCollector.cpp
@@ +55,5 @@
>  //
>  // A non-nsISupports ("native") object is scan-safe by explicitly
>  // providing its nsCycleCollectionParticipant.
>  //
>  // An object is purple-safe if it satisfies the following properties:

I don't know if it matters if you update the comment, but doesn't this mean that purple-safe is just the same thing as scan-safe? :)

@@ +201,5 @@
>      if (!ptr)
>          MOZ_CRASH();
>  }
>  
> +class AsyncKillSnowWhite : public nsRunnable

This should go down near where it is used.

@@ +786,5 @@
>          Visit(nsPurpleBuffer &aBuffer, nsPurpleBufferEntry *aEntry)
>          {
> +            if (aEntry->mRefCnt) {
> +                aEntry->mRefCnt->RemoveFromPurpleBuffer();
> +                aEntry->mRefCnt = nullptr;

This part at least is much simpler!

@@ +845,5 @@
>          MOZ_ASSERT(mCount != 0, "must have entries");
>  
> +        if (e->mRefCnt) {
> +            e->mRefCnt->RemoveFromPurpleBuffer();
> +            e->mRefCnt = nullptr;

It would make me feel better if you also did |e->mObject = nullptr;|, to avoid the possibility of use-after-free errors if you forget to nullcheck mRefCnt somewhere. Oh, but it looks like |mRefCnt == nullptr && mObject != nullptr| means the object is alive still, and just waiting for the CC to kill it.  Never mind then.

@@ +2195,5 @@
> +            aBuffer.Remove(aEntry);
> +        }
> +    }
> +
> +    nsTArray<SnowWhiteObject> mObjects;

presumably mObjects should be private.

@@ +3176,5 @@
>      timeLog.Checkpoint("ForgetSkippable()");
>  }
>  
>  void
> +nsCycleCollector_killSnowWhite(bool aAsync)

It looks like you only ever pass "false" from within nsCycleCollector.cpp, so I think it would make sense to split this up a bit.  First, make something like nsCycleCollector_dispatchDeferredDeletion() and put it in place of _killSnowWhite(true).

For the false case, CollectWhite could just call directly into KillSnowWhite(), and AsyncKillSnowWhite() could just do the |sCollector.get(); collector->KillSnowWhite()| thing directly, maybe?

::: xpcom/base/nsCycleCollector.h
@@ +42,5 @@
>  void nsCycleCollector_setForgetSkippableCallback(CC_ForgetSkippableCallback aCB);
>  
>  void nsCycleCollector_forgetSkippable(bool aRemoveChildlessNodes = false);
>  
> +void nsCycleCollector_killSnowWhite(bool aAsync);

This isn't a very good name for the function.  First of all, it sounds like you want to kill Snow White, the character from the fairy tales.  Secondly, it doesn't really describe what is happening.  Something like "_doDeferredDestruction" or something maybe. Likewise with the other places SnowWhite shows up.  Though I do feel bad killing off your cool names for things.

Comment 22

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

(In reply to Andrew McCreight [:mccr8] from comment #21)

> It would make me feel better if you also did |e->mObject = nullptr;|, to
> avoid the possibility of use-after-free errors if you forget to nullcheck
> mRefCnt somewhere.
mNextInFreeList is set below, and mNextInFreeList is union with mObject

>First, make
> something like nsCycleCollector_dispatchDeferredDeletion() and put it in
> place of _killSnowWhite(true).
Sounds ok.

Comment 23

[Andrew McCreight [:mccr8]]

Comment on attachment 763286 [details] [diff] [review]
v1.14

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

Here's another batch of random comments.  Sorry this is taking me so long, I've been a bit scattered brained this week.

::: xpcom/glue/nsISupportsImpl.h
@@ +98,2 @@
>  struct nsPurpleBufferEntry {
>    // mObject is set to null when nsCycleCollectingAutoRefCnt loses its

This comment needs to be updated.

@@ +110,5 @@
>  };
>  
> +#define NS_IN_PURPLE_BUFFER (1 << 0)
> +#define NS_IS_PURPLE (1 << 1)
> +#define NS_REFCOUNT_CHANGE (1 << 2)

Maybe you could define a constant for the number of bits you are using (2) and then use that in NS_REFCOUNT_CHANGE, NS_REFCOUNT_VALUE and the constructor? That would be a little clearer.

Another thing to consider is making these macros into private consts or something inside the ref count class itself, so they aren't defined everywhere, when you really just need them inside the class.

@@ +134,3 @@
>    }
>  
>    MOZ_ALWAYS_INLINE void stabilizeForDeletion()

stabilizeForDeletion no longer stabilizes like it used to, where it was basically sticking at a particular refcount. Is that okay?  I guess we need to have balanced refcounts anyways, and the 1 refcount should keep it alive.

@@ +151,3 @@
>    {
> +    MOZ_ASSERT(get() > 0);
> +    if (!(mRefCntAndFlags & NS_IN_PURPLE_BUFFER)) {

Maybe do (!IsInPurpleBuffer()) instead here?  It would be easier to read, and hopefully the compiler can turn !!! into !.

@@ +172,2 @@
>    {
> +    mRefCntAndFlags &= ~(NS_IS_PURPLE | NS_IN_PURPLE_BUFFER);

Can this have an assertion about how either or both of those flags are set?

@@ +255,5 @@
>   * cycle-collected classes that use the purple buffer to avoid leaks.
>   */
>  
>  #define NS_IMPL_CC_NATIVE_ADDREF_BODY(_class)                                 \
>      MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt");                      \

I think here and in WITH_LAST_RELEASE that int32_t(mRefCnt) should be more like mRefCnt.get()?

@@ +257,5 @@
>  
>  #define NS_IMPL_CC_NATIVE_ADDREF_BODY(_class)                                 \
>      MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt");                      \
>      NS_ASSERT_OWNINGTHREAD_AND_NOT_CCTHREAD(_class);                          \
> +    nsrefcnt count = mRefCnt.incr();                                          \

I hope nothing important depends on this count being right.

@@ +497,5 @@
>    NS_LOG_ADDREF(this, count, #_class, sizeof(*this));                         \
>    return count;                                                               \
>  }
>  
>  #define NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_DESTROY(_class, _destroy)       \

It looks to me that you should eliminate this macro entirely, and just inline it into NS_IMPL_CYCLE_COLLECTING_RELEASE, as you aren't using the |_destroy| argument at all.   There are also another 3 or 4 classes that use NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_DESTROY that I assume should also be converted to NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_LAST_RELEASE?

Comment 24

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

Attachment: v.1.16

Added a test fix for browser_privatebrowsing_windowtitle.js
(That test fails locally with or without SnowWhite)

https://tbpl.mozilla.org/?tree=Try&rev=69ab7ea4a2f9

Comment 25

[Andrew McCreight [:mccr8]]

Comment on attachment 763286 [details] [diff] [review]
v1.14

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

Here's some more comments.  I've mostly looked over it all at this point, though maybe not in a very coherent fashion.

::: content/base/src/nsDocument.cpp
@@ +1571,1 @@
>    NS_LOG_RELEASE(this, count, "nsDocument");

Whoah, I didn't realize nsDocument implemented its own release!  Though I guess I reviewed this patch, I just forgot it messed with nsDocument::Release.  That probably needs I need to fix up ICC somewhere.

::: js/xpconnect/src/xpcprivate.h
@@ +2309,5 @@
>          return NS_PARTICIPANT_AS(nsXPCOMCycleCollectionParticipant,
>                                   &participant);
>        }
>      };
> +    void DeleteCycleCollectable() {}

Why does the DeleteCycleCollectable for XPCWN and XPCWJS not delete the object?  Are these methods just never called because we don't use the purple buffer for them?

::: xpcom/base/nsCycleCollector.cpp
@@ +803,5 @@
>  
>      // RemoveSkippable removes entries from the purple buffer if
>      // nsPurpleBufferEntry::mObject is null or if the object's
>      // nsXPCOMCycleCollectionParticipant::CanSkip() returns true or
> +    // if nsPurpleBufferEntry::mRefCtn->IsPurple() is false.

mRefCtn -> mRefCnt

@@ +2195,5 @@
> +            aBuffer.Remove(aEntry);
> +        }
> +    }
> +
> +    nsTArray<SnowWhiteObject> mObjects;

Oh, you do look at mObjects.  Maybe there could be a predicate that returns true if |mObject.Length() != 0| and then you could use that and make this field private.

@@ +2257,5 @@
> +
> +void
> +nsCycleCollector::KillSnowWhite()
> +{
> +    while (true) {

This seems potentially dangerous. We could end up with a chain where one object frees another objects, which frees another object, etc.  Each time around the loop we have to check the entire purple buffer.  Could we bound the number of loops?

@@ +3076,1 @@
>      if (collector == (nsCycleCollector*)1) {

Maybe you should reverse this test, and just do a Suspect and return if we haven't shutdown the CC yet, as that's the simpler case, rather than nesting the complex one. :)

@@ +3076,4 @@
>      if (collector == (nsCycleCollector*)1) {
> +        if (aRefCnt->get() == 0) {
> +            if (!aShouldDelete) {
> +                nsCycleCollectionParticipant* participant = cp;

You don't need this |participant| local var.

::: xpcom/build/FrozenFunctions.cpp
@@ +88,5 @@
>      &NS_StringGetIsVoid,
>      &NS_CStringSetIsVoid,
>      &NS_CStringGetIsVoid,
>  
> +    // these functions were added post 1.9, but then made obsolete

You could probably just delete this comment. ;)

::: xpcom/glue/nsISupportsImpl.h
@@ +288,5 @@
> +                   _class::NS_CYCLE_COLLECTION_INNERCLASS::GetParticipant(),     \
> +                   &shouldDelete);                                               \
> +    NS_LOG_RELEASE(this, count, #_class);                                        \
> +    if (count == 0) {                                                            \
> +        mRefCnt.incr();                                                          \

hmm this whole thing is a little odd, but I think I see why it has to be like this.

@@ +497,5 @@
>    NS_LOG_ADDREF(this, count, #_class, sizeof(*this));                         \
>    return count;                                                               \
>  }
>  
>  #define NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_DESTROY(_class, _destroy)       \

Oh, I see where _destroy is used, the diff confused me. So NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_LAST_RELEASE has some cleanup that runs immediately, then later does the actual destruction of the object, whereas NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_DESTROY doesn't do anything until later. Is there some particular reason for ever using WITH_DESTROY with something beside |delete this|?  It kind of feels like WITH_LAST_RELEASE with an empty _last is a more reasonable default than WITH_DESTROY.

@@ +526,5 @@
> +  nsISupports *base = NS_CYCLE_COLLECTION_CLASSNAME(_class)::Upcast(this);    \
> +  nsrefcnt count = mRefCnt.decr(base, &shouldDelete);                         \
> +  NS_LOG_RELEASE(this, count, #_class);                                       \
> +  if (count == 0) {                                                           \
> +      mRefCnt.incr();                                                         \

XXX I don't understand this incr/decr pair instead of stabilizeForDeletion. I suppose it hinges on what shouldDelete is.

Comment 26

[Andrew McCreight [:mccr8]]

I still need to figure out the shouldDelete stuff a little more.  The incr/decr is a little odd looking there, as I said.

Comment 27

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

So we need to protect that _last doesn't end up re-releasing to 0. That is why there is .incr.
And to reset that extra incr, we decr.

stabilizeForDeletion is for the stuff happening in dtor, _last happens before that.

Comment 28

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

(In reply to Andrew McCreight [:mccr8] from comment #25)
> > +    void DeleteCycleCollectable() {}
> 
> Why does the DeleteCycleCollectable for XPCWN and XPCWJS not delete the
> object?  Are these methods just never called because we don't use the purple
> buffer for them?
Right. SW is for stuff that ends up to pb.



> > +nsCycleCollector::KillSnowWhite()
> > +{
> > +    while (true) {
> 
> This seems potentially dangerous. We could end up with a chain where one
> object frees another objects, which frees another object, etc.  Each time
> around the loop we have to check the entire purple buffer.  Could we bound
> the number of loops?
Not always. We need to clear all the SW before CC runs.
But I agree it is potentially dangerous.


> Oh, I see where _destroy is used, the diff confused me. So
> NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_LAST_RELEASE has some cleanup that
> runs immediately, then later does the actual destruction of the object,
> whereas NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_DESTROY doesn't do anything
> until later. Is there some particular reason for ever using WITH_DESTROY
> with something beside |delete this|?  It kind of feels like
> WITH_LAST_RELEASE with an empty _last is a more reasonable default than
> WITH_DESTROY.
WITH_DESTROY may generate faster code, since release stays simpler

Comment 29

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

Attachment: v1.17

Next round.
Didn't make macros consts, since I was told having them in the class
may not work everywhere.

I made SnowWhite killing to happen more async. Let's see if try is happy.

https://tbpl.mozilla.org/?tree=Try&rev=6ec369fe7c1c

Comment 30

[Andrew McCreight [:mccr8]]

Comment on attachment 766864 [details] [diff] [review]
v1.17

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

Looks good! I'm just r-ing because I'd like to see the patch after the review comments and questions are addressed, and you've rebased to trunk.

I wonder if it would be a good idea to run make sure the snow white stuff is all cleared out at shutdown, even though we're not doing the shutdown CCs and GCs.  It would make things die a little more consistently that are freed near shutdown.

As we discussed in IRC, please file a followup for investigating getting rid of the shouldDelete stuff in opt builds.  That might let us make decr() a little cleaner when we care about perf and don't care about shutdown leaks.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js
@@ +46,5 @@
>        // ensure that the test is run after the titlebar has been updated
>        browser.addEventListener("pageshow", function () {
>          browser.removeEventListener("pageshow", arguments.callee, false);
>          executeSoon(function () {
> +          if (aWindow.document.title != expected_title) {

You should get a PB person to review these test changes, whenever you've figured that out.

::: content/base/src/nsContentIterator.cpp
@@ +202,5 @@
>   ******************************************************/
>  
>  NS_IMPL_CYCLE_COLLECTING_ADDREF(nsContentIterator)
> +NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_LAST_RELEASE(nsContentIterator,
> +                                                   LastRelease())

How did you decide that this needs a LastRelease() (and the other places where you added them)?  How can other people tell that they should do the same thing for their classes?

::: content/base/src/nsDocument.cpp
@@ +1579,1 @@
>      nsNodeUtils::LastRelease(this);

Does this need to be modified to be more like _WITH_LAST_RELEASE, with the shouldDelete, .incr(), .decr() kind of stuff?

::: editor/txmgr/tests/TestTXMgr.cpp
@@ +444,5 @@
>      // Notice that we don't check to see if we go past the end of the array.
>      // This is done on purpose since we want to crash if the order array is out
>      // of date.
>      //
> +    /* Disabled because the current cycle collector doesn't delete

You should get Ehsan or somebody who knows editor to review these changes, because I don't know if it is okay or not.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +561,5 @@
>      static NS_METHOD UnrootImpl(void *n)
>      {
>          return NS_OK;
>      }
> +    static NS_METHOD_(void) DeleteCycleCollectableImpl(void *p)

You are going to have to rebase these various XPConnect changes, but it should just affect what files they are in.

::: xpcom/base/nsAgg.h
@@ +167,3 @@
>      return count;                                                           \
> +}                                                                           \
> +                                                                            \

Please remove this blank line to make it clearer this is part of the previous macro.

::: xpcom/base/nsCycleCollector.cpp
@@ +85,3 @@
>  // either side of the calls to |Unlink|. This keeps the set of white
>  // objects alive during the unlinking.
>  //

Please add a comment somewhere in this file that explains what it means for an object to be snow white is (eg its reference count has dropped to zero, so it is really dead, and just waiting for deletion or something).

@@ +169,5 @@
>  #else
>  PRThread* gCycleCollectorThread = nullptr;
>  #endif
>  
> +struct SnowWhiteObject

You should move this down near SnowWhiteKiller where it is used.

@@ +770,5 @@
>          Visit(nsPurpleBuffer &aBuffer, nsPurpleBufferEntry *aEntry)
>          {
> +            if (aEntry->mRefCnt) {
> +                aEntry->mRefCnt->RemoveFromPurpleBuffer();
> +                aEntry->mRefCnt = nullptr;

You should probably also do |aEntry->mObject = nullptr|.

@@ +785,5 @@
>  
>      void SelectPointers(GCGraphBuilder &builder);
>  
>      // RemoveSkippable removes entries from the purple buffer if
>      // nsPurpleBufferEntry::mObject is null or if the object's

Can mObject ever be null now for an entry not in the free list?  Isn't clearing mObject just something you did in the previous round of purple buffer improvements?

You could probably remove a lot of null checks. But you can just file a followup for that, if appropriate. It shouldn't matter much.

@@ +877,5 @@
>      void
>      Visit(nsPurpleBuffer &aBuffer, nsPurpleBufferEntry *aEntry)
>      {
> +        MOZ_ASSERT(!(aEntry->mObject && !aEntry->mRefCnt->get()),
> +                   "SelectPointersVisitor: snow-white object in the purple buffer");

Does this assertion hold because we've already swept the purple buffer for snow-white objects?

@@ +878,5 @@
>      Visit(nsPurpleBuffer &aBuffer, nsPurpleBufferEntry *aEntry)
>      {
> +        MOZ_ASSERT(!(aEntry->mObject && !aEntry->mRefCnt->get()),
> +                   "SelectPointersVisitor: snow-white object in the purple buffer");
> +        if (aEntry->mObject && !aEntry->mRefCnt->IsPurple()) {

Both cases just do the same thing now, so it just combine them into one huge if statement.

@@ +1083,5 @@
>      // Start and finish an individual collection.
>      bool BeginCollection(ccType aCCType, nsICycleCollectorListener *aListener);
>      bool FinishCollection(nsICycleCollectorListener *aListener);
>  
> +    void KillSnowWhite(bool aUntilNoSWInPurpleBuffer);

Could you please call this (and other things like it in this file) FreeSnowWhite or DeleteSnowWhite instead of KillSnowWhite?  Snow White is a fairy tale character, and thus Kill Snow White sounds very violent. :)

@@ +2241,2 @@
>  {
> +    RemoveSkippableVisitor visitor(Count(), removeChildlessNodes, aCb);

Have you noticed that a large % of things in the purple buffer are snow white?

@@ +2269,5 @@
> +{
> +    do {
> +        SnowWhiteKiller visitor(mPurpleBuf.Count());
> +        mPurpleBuf.VisitEntries(visitor);
> +        if (!visitor.HasSnowWhiteObjects()) {

You could put this in the while condition but no big deal.

@@ +2272,5 @@
> +        mPurpleBuf.VisitEntries(visitor);
> +        if (!visitor.HasSnowWhiteObjects()) {
> +            break;
> +        }
> +    } while(aUntilNoSWInPurpleBuffer);

while (

::: xpcom/build/nsXPCOMPrivate.h
@@ +160,1 @@
>      CycleCollectorFunc cycleSuspectFunc; // obsolete: use cycleSuspect2Func

You should update this reference to cycleSuspect2Func.

@@ +164,5 @@
>      CStringSetIsVoidFunc cstringSetIsVoid;
>      CStringGetIsVoidFunc cstringGetIsVoid;
>  
>      // Added for Mozilla 1.9.1
> +    CycleCollectorSuspect2Func cycleSuspect2Func; // obsolete

Probably a good idea to say to use cycleSuspect3Func here in the comment.

::: xpcom/glue/nsISupportsImpl.h
@@ +91,4 @@
>    nsrefcnt(NS_PTR_TO_INT32(tagged_) >> 1)
>  #define NS_CCAR_TAGGED_TO_PURPLE_ENTRY(tagged_) \
>    static_cast<nsPurpleBufferEntry*>(tagged_)
>  #define NS_CCAR_TAGGED_STABILIZED_REFCNT NS_CCAR_PURPLE_ENTRY_TO_TAGGED(0)

You should be able to delete a bunch of these macros now, maybe even all of them.

@@ +107,5 @@
>  };
>  
> +#define NS_IN_PURPLE_BUFFER (1 << 0)
> +#define NS_IS_PURPLE (1 << 1)
> +#define NS_REFCOUNT_CHANGE (1 << 2)

Please use a #define or something for the 2 here and in NS_REFCOUNT_VALUE and the constructor, with a name about the number of flags or something.

@@ +119,3 @@
>    {}
>  
> +  nsCycleCollectingAutoRefCnt(uintptr_t aValue)

Can you get rid of this constructor?  It doesn't look like it is used anywhere.

@@ +150,5 @@
> +    if (!IsInPurpleBuffer()) {
> +      mRefCntAndFlags -= NS_REFCOUNT_CHANGE;
> +      mRefCntAndFlags |= (NS_IN_PURPLE_BUFFER | NS_IS_PURPLE);
> +      uintptr_t retval = NS_REFCOUNT_VALUE(mRefCntAndFlags);
> +      NS_CycleCollectorSuspect3(owner, p, this, shouldDelete);

It would probably be a good idea to add a comment that |this| may be deleted or unpurpled when we return from Suspect, as this chunk of code really makes it look tempting to refactor out and share code on the two paths.

@@ +490,5 @@
>  {                                                                             \
>    MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt");                        \
>    NS_ASSERT_OWNINGTHREAD_AND_NOT_CCTHREAD(_class);                            \
>    nsrefcnt count =                                                            \
> +    mRefCnt.incr();                                                           \

Please put this incr on the same line as count.

@@ +507,3 @@
>    return count;                                                               \
> +}                                                                             \
> +                                                                              \

Could you get rid of this blank line in between Release and DeleteCycleCollectable?  I keep forgetting they are part of a single macro and end up thinking _destroy isn't used.

@@ +514,5 @@
>  
>  #define NS_IMPL_CYCLE_COLLECTING_RELEASE(_class)                              \
>    NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_DESTROY(_class, delete (this))
>  
> +#define NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_LAST_RELEASE(_class, _last)     \

There should be some comment describing what this is for and when you might want to use it.

@@ +534,5 @@
> +      }                                                                       \
> +  }                                                                           \
> +  return count;                                                               \
> +}                                                                             \
> +                                                                              \

Same here.

Comment 31

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

> >  NS_IMPL_CYCLE_COLLECTING_ADDREF(nsContentIterator)
> > +NS_IMPL_CYCLE_COLLECTING_RELEASE_WITH_LAST_RELEASE(nsContentIterator,
> > +                                                   LastRelease())
> 
> How did you decide that this needs a LastRelease() (and the other places
> where you added them)?  How can other people tell that they should do the
> same thing for their classes?
It took awhile to realize we create lots of contentitators in editor code, and those contentiterators keep
range objects alive and that ends up slowing certain editor test, which is doing tons of stuff within one
task.
I realized that editor may actually slow down DOM performance even without SW by keeping lots of Range
objects alive. 
Anyhow, that is very special case.

> >      nsNodeUtils::LastRelease(this);
> 
> Does this need to be modified to be more like _WITH_LAST_RELEASE, with the
> shouldDelete, .incr(), .decr() kind of stuff?
No. LastRelease is last release. delete will be called afterwards.

> ::: editor/txmgr/tests/TestTXMgr.cpp
> @@ +444,5 @@
> >      // Notice that we don't check to see if we go past the end of the array.
> >      // This is done on purpose since we want to crash if the order array is out
> >      // of date.
> >      //
> > +    /* Disabled because the current cycle collector doesn't delete
> 
> You should get Ehsan or somebody who knows editor to review these changes,
> because I don't know if it is okay or not.
Well, the test is 10+ years old

> >      Visit(nsPurpleBuffer &aBuffer, nsPurpleBufferEntry *aEntry)
> >      {
> > +        MOZ_ASSERT(!(aEntry->mObject && !aEntry->mRefCnt->get()),
> > +                   "SelectPointersVisitor: snow-white object in the purple buffer");
> 
> Does this assertion hold because we've already swept the purple buffer for
> snow-white objects?
yes.

> >      // Start and finish an individual collection.
> >      bool BeginCollection(ccType aCCType, nsICycleCollectorListener *aListener);
> >      bool FinishCollection(nsICycleCollectorListener *aListener);
> >  
> > +    void KillSnowWhite(bool aUntilNoSWInPurpleBuffer);
> 
> Could you please call this (and other things like it in this file)
> FreeSnowWhite or DeleteSnowWhite instead of KillSnowWhite?  Snow White is a
> fairy tale character, and thus Kill Snow White sounds very violent. :)



> 
> @@ +2241,2 @@
> >  {
> > +    RemoveSkippableVisitor visitor(Count(), removeChildlessNodes, aCb);
> 
> Have you noticed that a large % of things in the purple buffer are snow
> white?
I'll check, but it depends a lot on the case.

> You could put this in the while condition but no big deal.
I actually can't since the visitor is defined in the do-while.


> @@ +119,3 @@
> >    {}
> >  
> > +  nsCycleCollectingAutoRefCnt(uintptr_t aValue)
> 
> Can you get rid of this constructor?  It doesn't look like it is used
> anywhere.
followup? I just keep the old API there.

Comment 32

[Andrew McCreight [:mccr8]]

Ok, that all sounds good to me.

Comment 33

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

Attachment: v.1.26 (aka 1.17 + fixing comments)

ehsan gave r+ to editor/ and pb test fixes.

(But need to still figure out some other pb failures.)

Comment 34

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

Comment on attachment 768601 [details] [diff] [review]
v.1.26 (aka 1.17 + fixing comments)

Er, I need to still update my tree and merge.

Comment 35

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

Attachment: 1.27

This should compile

Comment 36

[Andrew McCreight [:mccr8]]

Comment on attachment 768619 [details] [diff] [review]
1.27

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

Pretty cool, thanks.

::: xpcom/glue/nsISupportsImpl.h
@@ +98,5 @@
> +#define NS_NUMBER_OF_FLAGS_IN_REFCNT 2
> +#define NS_IN_PURPLE_BUFFER (1 << 0)
> +#define NS_IS_PURPLE (1 << 1)
> +#define NS_REFCOUNT_CHANGE (1 << NS_NUMBER_OF_FLAGS_IN_REFCNT)
> +#define NS_REFCOUNT_VALUE(_val) (_val >> 2)

nit: the 2 in NS_REFCOUNT_VALUE should be NS_NUMBER_OF_FLAGS_IN_REFCNT

Comment 37

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

Attachment: 1.27b

Comment 38

[Andrew McCreight [:mccr8]]

This might just be an artifact of ICC, but you might want to add a call to     |FreeSnowWhite(true);| to the end of FixGrayBits, because running the GC there can free objects, which will cause more snow-white objects to appear. Then you can remove the FreeSnowWhite() call from ShutdownCollect().  The explicit call to FreeSnowWhite() covers you for the shutdown collect, but if we do end up triggering FixGrayBits during a normal collection, then we can end up freeing objects after we've done FreeSnowWhite() and end up with SnowWhite objects in the purple buffer when we SelectPointers.  I think...

Comment 39

[Andrew McCreight [:mccr8]]

Thinking more about this, I'm more confident in my analysis in comment 38.  In general, it makes me a little nervous that this has to depend so heavily on all the snow-white being purged before we build the graph, but hopefully there's no other weird path that can cause objects to be freed.  In ICC I had to fix up a few paths to make sure snow-white was always purged.

Comment 40

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

FreeSnowWhite(true); is always called after FixGrayBits, and if mCollector->PrepareForCollection(aResults, &whiteNodes) returns false, I don't want
to FreeSnowWhite.

Comment 41

[Andrew McCreight [:mccr8]]

You could make SnowWhiteKiller::HasSnowWhiteObjects() const

Comment 42

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

Attachment: pb test fixes

Not pretty, but seems to be working.
Need to make sure we have properly loaded/activated/delayLoaded new browser windows. 
Need to also close stuff early enough, or otherwise the page we're trying to
focus right after the page, might not be the right one.

delayedLoad is a left-over from an earlier attempt to fix this stuff.

Seeking for an rs+ ;)

Comment 43

[(no longer active)]

Comment on attachment 772643 [details] [diff] [review]
pb test fixes

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

Yikes!

Comment 44

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

Attachment: v.1.61

Comment 45

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d42a2a82f3d2

Comment 46

[Ryan VanderMeulen [:RyanVM]]

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