Bug 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

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla25
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 12 obsolete attachments)

87.22 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
87.39 KB, patch
Details | Diff | Splinter Review
27.02 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
116.32 KB, patch
Details | Diff | Splinter Review
Assignee

Description

7 years ago
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.
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.
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.
Assignee

Comment 3

7 years ago
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.
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.
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.
Assignee

Comment 6

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

Updated

7 years ago
Depends on: 798158
Assignee

Updated

7 years ago
Alias: snow-white
Assignee

Updated

6 years ago
Assignee: nobody → bugs
Assignee

Updated

6 years ago
Blocks: 866681
Assignee

Comment 7

6 years ago
Posted patch v1 (obsolete) — Splinter Review
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
Assignee

Comment 8

6 years ago
Posted patch v1.1 (obsolete) — Splinter Review
Maybe this compiles on 64bit debug osx
https://tbpl.mozilla.org/?tree=Try&rev=bbf1b42b4a4a
Assignee

Comment 9

6 years ago
ok, "tiny" bit crashy on try :/
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.
Assignee

Comment 11

6 years ago
Posted patch v1.3 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=05cafa4e4115
Attachment #761061 - Attachment is obsolete: true
Attachment #761114 - Attachment is obsolete: true
Assignee

Comment 12

6 years ago
Posted patch v1.4 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=9279d8711b0d

Better to kill snow white at the right time.
Attachment #761345 - Attachment is obsolete: true
Assignee

Comment 13

6 years ago
Posted patch v1.5 (obsolete) — Splinter Review
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.
Attachment #761400 - Attachment is obsolete: true
Assignee

Comment 14

6 years ago
Posted patch v1.6 (obsolete) — Splinter Review
Try to kill snow white more often
https://tbpl.mozilla.org/?tree=Try&rev=5563557e6d4f
Attachment #761456 - Attachment is obsolete: true
Assignee

Comment 15

6 years ago
Posted patch v1.8 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=65fc0b898c56
Attachment #761704 - Attachment is obsolete: true
Assignee

Comment 16

6 years ago
Posted patch v1.13 (obsolete) — Splinter Review
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.
Attachment #762592 - Attachment is obsolete: true
Attachment #763163 - Flags: review?(continuation)
Assignee

Comment 17

6 years ago
(That is a kind of feedback/review? )
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?
Assignee

Comment 19

6 years ago
Posted patch v1.14 (obsolete) — Splinter Review
small changes
Attachment #763163 - Attachment is obsolete: true
Attachment #763163 - Flags: review?(continuation)
Assignee

Comment 20

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

There is probably still one issue lurking somewhere in devtools.
Attachment #763286 - Flags: feedback?(continuation)
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.
Assignee

Comment 22

6 years ago
(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 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?
Assignee

Comment 24

6 years ago
Posted patch v.1.16 (obsolete) — Splinter Review
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 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.
Attachment #763286 - Attachment is obsolete: true
Attachment #763286 - Flags: feedback?(continuation)
I still need to figure out the shouldDelete stuff a little more.  The incr/decr is a little odd looking there, as I said.
Assignee

Comment 27

6 years ago
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.
Assignee

Comment 28

6 years ago
(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
Assignee

Comment 29

6 years ago
Posted patch v1.17 (obsolete) — Splinter Review
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
Attachment #765641 - Attachment is obsolete: true
Attachment #766864 - Flags: review?(continuation)
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.
Attachment #766864 - Flags: review?(continuation) → review-
Assignee

Updated

6 years ago
Depends on: 887879
Assignee

Comment 31

6 years ago
> >  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.
No longer depends on: 887879
Ok, that all sounds good to me.
Assignee

Comment 33

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

(But need to still figure out some other pb failures.)
Attachment #766864 - Attachment is obsolete: true
Attachment #768601 - Flags: review?(continuation)
Assignee

Comment 34

6 years ago
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.
Attachment #768601 - Flags: review?(continuation)
Assignee

Comment 35

6 years ago
Posted patch 1.27Splinter Review
This should compile
Attachment #768619 - Flags: review?(continuation)
Blocks: 887879
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
Attachment #768619 - Flags: review?(continuation) → review+
Attachment #768601 - Attachment is obsolete: true
Assignee

Comment 37

6 years ago
Posted patch 1.27bSplinter Review
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...
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.
Assignee

Comment 40

6 years ago
FreeSnowWhite(true); is always called after FixGrayBits, and if mCollector->PrepareForCollection(aResults, &whiteNodes) returns false, I don't want
to FreeSnowWhite.
Blocks: 888999
You could make SnowWhiteKiller::HasSnowWhiteObjects() const
Assignee

Comment 42

6 years ago
Posted patch pb test fixesSplinter Review
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+ ;)
Attachment #772643 - Flags: review?(ehsan)

Comment 43

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

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

Yikes!
Attachment #772643 - Flags: review?(ehsan) → review+
Assignee

Comment 44

6 years ago
Posted patch v.1.61Splinter Review
https://hg.mozilla.org/mozilla-central/rev/d42a2a82f3d2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

6 years ago
Blocks: 798158
No longer depends on: 798158
Depends on: 892315

Updated

6 years ago
Depends on: 901448

Updated

6 years ago
Depends on: 918223
Depends on: 894914
Depends on: 892630
No longer depends on: 892630
You need to log in before you can comment on or make changes to this bug.