716518 - Add beforeTraverse and purpleCleanUp phases to CC

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

Attachment: patch

The patch is pretty much brute-force. 
Adds also RemovePurple() to nsCycleCollectingAutoRefCnt.

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 586966 [details] [diff] [review]
patch

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

This is a great framework for adding new optimizations to the cycle collector!  It still needs some fixing, but the basic shape is good.

I have a lot of comments, but they mostly fall into a few categories:
- Clarifications where I want to make sure I understand what you are doing.
- The names of some things could be improved.
- The return values of the live predicates don't seem to be handled consistently, and maybe could be bools.

::: xpcom/base/nsCycleCollector.cpp
@@ +865,5 @@
>          }
>      }
>  
>      void SelectPointers(GCGraphBuilder &builder);
> +    void MaybeClear();

This name is vague.  RemoveAlwaysLive maybe? (The purple buffer class seems to use "remove" for removing things.)

@@ +1086,5 @@
>  
>      nsPurpleBuffer mPurpleBuf;
>  
> +    CC_BeforeUnlinkCallback mBeforeUnlinkCB;
> +    CC_BeforeUnlinkCallback mClearPurpleCB;

mClearPurpleCB has the "wrong" type.

@@ +1098,5 @@
>      void MarkRoots(GCGraphBuilder &builder);
>      void ScanRoots();
>      void ScanWeakMaps();
>  
> +    void MaybeClearPurple();

Again, this is vague and should be updated to match MaybeClear above.  RemoveAlwaysLivePurple() or something.

@@ +1909,5 @@
>                   "Don't add objects that don't participate in collection!");
>  
>      nsXPCOMCycleCollectionParticipant *cp;
>      ToParticipant(root, &cp);
>  

This will add the node even if BeforeTraverse fails, which seems a little odd to me.

@@ +1926,5 @@
>  
> +void
> +nsPurpleBuffer::MaybeClear()
> +{
> +    // Walk through all the blocks.

I think we need some kind of iterator over real purple buffer entries, but this is already done in three places, so we can deal with it in a separate bug.

@@ +1939,5 @@
> +                if (e->mObject) {
> +                    nsISupports* o = canonicalize(e->mObject);
> +                    nsXPCOMCycleCollectionParticipant* cp;
> +                    ToParticipant(o, &cp);
> +                    nsresult rv = cp->MaybeClearPurple(o);

Control flow here is a little complex.  Maybe something like
  if (rv == NS_OK)
    continue;
  if (rv != NS_SUCCESS_INTERRUPTED_TRAVERSE)
    return;
  cp->UnmarkPurple(o);
...then you can get rid of freeEntry altogether?

It also seems weird to me that if a MaybeClearPurple fails, then we don't try to clear any other entries, but we otherwise continue with the CC as if nothing happened.  Should we treat failure the same as NS_OK, or maybe bail out harder if a maybeclear fails?

@@ +1946,5 @@
> +                    }
> +                    if ((freeEntry = (rv == NS_SUCCESS_INTERRUPTED_TRAVERSE))) {
> +                        cp->UnmarkPurple(o);
> +                    }
> +                }

What is this freeEntry section doing?  Things that are unpurpled outside of the CC itself are NULL'ed out but not added to the free list? (also newly unpurpled INTERRUPTED things).

@@ +1953,5 @@
> +                    mNormalObjects.RemoveEntry(e->mObject);
> +#endif
> +                    --mCount;
> +                    e->mNextInFreeList = (nsPurpleBufferEntry*)
> +                        (PRUword(mFreeList) | PRUword(1));

All this gross bit-twiddling of buffer entry pointers here and elsewhere should probably get turned into functions in some later bug, but this is fine for this bug.

@@ +2000,5 @@
> +{
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    if (obs) {
> +        obs->NotifyObservers(nsnull, "cycle-collector-purple-cleanup", nsnull);
> +    }

Do we only return to this function until after all of the relevant observers have run to completion?

@@ +3807,5 @@
> +    }
> +}
> +
> +void
> +nsCycleCollector_maybeClearPurple()

This name should be fixed along with the others.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +159,4 @@
>      void NS_COM_GLUE TraverseScriptObjects(void *p,
>                                          nsCycleCollectionTraversalCallback &cb);
>  };
>  

mNeedsBeforeTraverse is a per-class property, right?  Like, nsGenericElements need it, so it will be set to true there, but nsFooBar doesn't, so it will be set to false there?

@@ +166,5 @@
>  public:
> +    nsXPCOMCycleCollectionParticipant() : mNeedsBeforeTraverse(false) {}
> +    nsXPCOMCycleCollectionParticipant(bool aNBT)
> +    : mNeedsBeforeTraverse(aNBT) {}
> +

This use of a constant to avoid virtual method calls is a little hair-raising to me.  Can we somehow check that if mNeedsBeforeTraverse is false then the "Real" functions below have not been overloaded?  Though that seems kind of crazy, so probably not.

Have you actually measured what the overhead of using normal virtual methods would be?  I think PGO can do some trickery to speed up common cases, too.

@@ +178,5 @@
>  
>      NS_IMETHOD_(void) UnmarkPurple(nsISupports *p);
>  
>      bool CheckForRightISupports(nsISupports *s);
> +

Should these both should MOZ_ASSERT false in addition to returning NS_OK?  It seems like if we are inside the default virtual method, then somebody is accessing things incorrectly, or initializing mNeedsBeforeTraverse incorrectly.  Could we make the "real" functions protected?  Though I did read somewhere that "protected" was a bad thing.

@@ +181,5 @@
>      bool CheckForRightISupports(nsISupports *s);
> +
> +    NS_IMETHOD BeforeTraverseReal(void *p) { return NS_OK; }
> +    NS_IMETHOD MaybeClearPurpleReal(void *p) { return NS_OK; }
> +

The difference between BeforeTraverse and MaybeClearPurple is that the former is always called in the cycle collector itself, whereas the latter is called only in nsCycleCollector_maybeClearPurple?  There should be some comment to this effect. I see that nsGenericElement has different implementations of these in your full patch, but they look fairly similar at a glance.  What sort of things are going to be different between these two functions?

"BeforeTraverseReal" and "MaybeClearPurpleReal" don't really describe what the functions do, and should probably be more similar given how similar the functions are.  Could these be changed to something like "MustBeLiveBeforeTraverse" and "MustBeLive"?  "Always" instead of "MustBe" maybe?

It also seems a little weird to me that they return an nsresult and not a boolean that says if the object must really be alive.  So if a node is in a live document, then it would return "true", indicating that it must be alive?  But maybe this is some XPCOM-ism I don't understand.  Do you think these functions will ever return anything besides NS_OK and NS_INTERRUPTED_TRAVERSE?  I guess if they can ever fail in some way, then we want an nsResult.  But on the other hand, we don't really do much with a failure, so we could just return "false" in that case...

@@ +197,5 @@
> +            return MaybeClearPurpleReal(p);
> +        }
> +        return NS_OK;
> +    }
> +

I think mNeedsBeforeTraverse should be private.

@@ +679,5 @@
>    }                                                                            \
>  
>  #define NS_IMPL_CYCLE_COLLECTION_CLASS(_class)                                 \
>    NS_CYCLE_COLLECTION_CLASSNAME(_class) NS_CYCLE_COLLECTION_NAME(_class);
> +  

Can you define this macro as NS_IMPL_CYCLE_COLLECTION_CLASS(_class)?  So it is more obvious you are just doing the same thing.

::: xpcom/glue/nsISupportsImpl.h
@@ +224,5 @@
>      NS_ASSERTION(IsPurple(), "must be purple");
>      nsrefcnt refcount = NS_CCAR_TAGGED_TO_PURPLE_ENTRY(mTagged)->mRefCnt;
>      mTagged = NS_CCAR_REFCNT_TO_TAGGED(refcount);
>    }
>  

This is unused.  Maybe it makes sense to put it into a later patch?  I'm not sure how exactly it is used.  It overlaps a lot with unmarkPurple.

Comment 2

[Andrew McCreight [:mccr8]]

Feel free to put off the name changing stuff to a later revision of this patch, after we figure out what would be good, and maybe get somebody else's input, as it will probably be annoying to change it everywhere...

Comment 3

[Andrew McCreight [:mccr8]]

> This is unused.
That was supposed to be for RemovePurple.

Comment 4

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

RemovePurple will be used in a different bug.
I added that part here, since it is relevant to the purple buffer usage.

Comment 5

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

(In reply to Andrew McCreight [:mccr8] from comment #1)
> >      void SelectPointers(GCGraphBuilder &builder);
> > +    void MaybeClear();
> 
> This name is vague.  RemoveAlwaysLive maybe? (The purple buffer class seems
> to use "remove" for removing things.)
Would "TryRemoveAlwaysLive" work?
Since it really depends on the stuff outside cycle collector what the method does.
It can be no-op.


> 
> @@ +1086,5 @@
> >  
> >      nsPurpleBuffer mPurpleBuf;
> >  
> > +    CC_BeforeUnlinkCallback mBeforeUnlinkCB;
> > +    CC_BeforeUnlinkCallback mClearPurpleCB;
> 
> mClearPurpleCB has the "wrong" type.
Oops.


> @@ +1909,5 @@
> >                   "Don't add objects that don't participate in collection!");
> >  
> >      nsXPCOMCycleCollectionParticipant *cp;
> >      ToParticipant(root, &cp);
> >  
> 
> This will add the node even if BeforeTraverse fails, which seems a little
> odd to me.
How so? The idea is that BeforeTraverse needs to explicitly return some particular value to
prevent adding stuff to the graph. In all other cases the object will be in the graph.


> @@ +1939,5 @@
> > +                if (e->mObject) {
> > +                    nsISupports* o = canonicalize(e->mObject);
> > +                    nsXPCOMCycleCollectionParticipant* cp;
> > +                    ToParticipant(o, &cp);
> > +                    nsresult rv = cp->MaybeClearPurple(o);
> 
> Control flow here is a little complex.  Maybe something like
>   if (rv == NS_OK)
>     continue;
>   if (rv != NS_SUCCESS_INTERRUPTED_TRAVERSE)
>     return;
>   cp->UnmarkPurple(o);
> ...then you can get rid of freeEntry altogether?
Ok

> 
> It also seems weird to me that if a MaybeClearPurple fails, then we don't
> try to clear any other entries, but we otherwise continue with the CC as if
> nothing happened.  Should we treat failure the same as NS_OK, or maybe bail
> out harder if a maybeclear fails?
Well, currently MaybeClearPurple doesn't fail. It just returns NS_OK or NS_SUCCESS_INTERRUPTED_TRAVERSE
I can change this so that failure means continue in the loop


> > +                    }
> > +                    if ((freeEntry = (rv == NS_SUCCESS_INTERRUPTED_TRAVERSE))) {
> > +                        cp->UnmarkPurple(o);
> > +                    }
> > +                }
> 
> What is this freeEntry section doing?
>  Things that are unpurpled outside of
> the CC itself are NULL'ed out but not added to the free list? 
Right

> > +{
> > +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> > +    if (obs) {
> > +        obs->NotifyObservers(nsnull, "cycle-collector-purple-cleanup", nsnull);
> > +    }
> 
> Do we only return to this function until after all of the relevant observers
> have run to completion?
NotifyObservers is sync if you ask that.

> 
> mNeedsBeforeTraverse is a per-class property, right?
Per CC object.
>  Like,
> nsGenericElements need it, so it will be set to true there, but nsFooBar
> doesn't, so it will be set to false there?
Right. there is just one CC object for nsGenericElements


> 
> This use of a constant to avoid virtual method calls is a little
> hair-raising to me.  Can we somehow check that if mNeedsBeforeTraverse is
> false then the "Real" functions below have not been overloaded?  Though that
> seems kind of crazy, so probably not.
We use similar tricks elsewhere to avoid *slow* virtual calls.
I don't see any problem with this.


> Have you actually measured what the overhead of using normal virtual methods
> would be?  I think PGO can do some trickery to speed up common cases, too.
In some cases yes, but with CC there are tons of different implementations.

> 
> Should these both should MOZ_ASSERT false in addition to returning NS_OK? 
> It seems like if we are inside the default virtual method, then somebody is
> accessing things incorrectly, or initializing mNeedsBeforeTraverse
> incorrectly.  Could we make the "real" functions protected?
Ah, we could

>  Though I did
> read somewhere that "protected" was a bad thing.
really?

> 
> The difference between BeforeTraverse and MaybeClearPurple is that the
> former is always called in the cycle collector itself, whereas the latter is
> called only in nsCycleCollector_maybeClearPurple?
Yes

 > There should be some
> comment to this effect. I see that nsGenericElement has different
> implementations of these in your full patch, but they look fairly similar at
> a glance.  What sort of things are going to be different between these two
> functions?
Since BeforeTraverse traverse doesn't run in main thread, it can't do certain things.
MaybeClearPurple can for example unmark_gray

> 
> "BeforeTraverseReal" and "MaybeClearPurpleReal" don't really describe what
> the functions do, and should probably be more similar given how similar the
> functions are.  Could these be changed to something like
> "MustBeLiveBeforeTraverse" and "MustBeLive"?  "Always" instead of "MustBe"
> maybe?
No. Since cycle collector just calls some helpers (which may or may not be implemented)



> It also seems a little weird to me that they return an nsresult and not a
> boolean that says if the object must really be alive.
I could change them to return boolean.

> Can you define this macro as NS_IMPL_CYCLE_COLLECTION_CLASS(_class)?  So it
> is more obvious you are just doing the same thing.
Don't understand this

> 
> This is unused.  Maybe it makes sense to put it into a later patch?  I'm not
> sure how exactly it is used.  It overlaps a lot with unmarkPurple.
unmarkPurple doesn't set the buffer to null

Comment 6

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

Actually, I'd prefer to not return boolean, since returning 
NS_SUCCESS_INTERRUPTED_TRAVERSE makes it way more easier to understand what is happening.

I could ofc create an enum { eCanBeRemovedFromPurpleBuffer, eKeepInPurpleBuffer }

Comment 7

[Andrew McCreight [:mccr8]]

For the return value for the two callbacks, nsresult doesn't seem quite right to me.  NS_OK doesn't really mean "leave in purple buffer" to me right off the bat.  The interrupted traverse value makes some sense if you are familiar with Traverse functions, but on the other hand, we're not really calling these functions during traversal.  Finally, there are all of these other values it can return, and it isn't clear what that should mean.  What if we return an error?  Should we do something about it?  What if we return something else entirely?  Do we treat that the same as an error or like NS_OK or what?

It seems to me there are only two kinds of values we care about.  Either the function determines that the CC doesn't need to visit the object (because it is live or acyclic or what have you), or we don't know, so we have to visit it to find out.  I think a boolean is fine, if the name of the two callbacks better describe what they are supposed to be doing.  For instance, if they were called CanSkipTraverse() then you would write something like
  if (foo->CanSkipTraverse()) { removeFromPurpleBuffer(); }
and it is clear, to me at least, what is happening.  And writing a little function called CanSkipTraverse(), it seems clear to me what returning true or false is.  Does that sound reasonable to you?

Comment 8

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

(In reply to Andrew McCreight [:mccr8] from comment #7)
>   I think a boolean is fine, if the name of the two callbacks
> better describe what they are supposed to be doing.  For instance, if they
> were called CanSkipTraverse() then you would write something like
>   if (foo->CanSkipTraverse()) { removeFromPurpleBuffer(); }
> and it is clear, to me at least, what is happening.  And writing a little
> function called CanSkipTraverse(), it seems clear to me what returning true
> or false is.  Does that sound reasonable to you?
Sounds ok

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #5)
> > This name is vague.  RemoveAlwaysLive maybe? (The purple buffer class seems
> > to use "remove" for removing things.)
> Would "TryRemoveAlwaysLive" work?
> Since it really depends on the stuff outside cycle collector what the method
> does. It can be no-op.

I think if the name aligns with the callbacks it is okay to not be 100% descriptive in the name about how we may not actually remove everything that is live.  Just add a comment saying that it removes everything in the buffer that returns true for CanSkipTraverse or whatever.  (then this could be RemoveSkippable?)

(I discussed the return value issue which comes up in a bunch of places in your reply, in comment 7.)

> > What is this freeEntry section doing?
> >  Things that are unpurpled outside of
> > the CC itself are NULL'ed out but not added to the free list? 
> Right

Is that a new thing or could that happen before?  If the former, it should probably be documented in a comment somewhere.

> > Do we only return to this function until after all of the relevant observers
> > have run to completion?
> NotifyObservers is sync if you ask that.
Okay, thanks.

> > This use of a constant to avoid virtual method calls is a little
> > hair-raising to me.  Can we somehow check that if mNeedsBeforeTraverse is
> > false then the "Real" functions below have not been overloaded?  Though that
> > seems kind of crazy, so probably not.
> We use similar tricks elsewhere to avoid *slow* virtual calls.
> I don't see any problem with this.
Okay, if it is a standard thing, then that's fine.

> >  Though I did
> > read somewhere that "protected" was a bad thing.
> really?
Well, looks like we use it all over the place, so I suppose it is okay.  I'm not a C++ expert so I don't have a personal opinion.
 
>  > There should be some
> > comment to this effect. I see that nsGenericElement has different
> > implementations of these in your full patch, but they look fairly similar at
> > a glance.  What sort of things are going to be different between these two
> > functions?
> Since BeforeTraverse traverse doesn't run in main thread, it can't do
> certain things.
> MaybeClearPurple can for example unmark_gray
Ah, that makes sense.  There should be comments describing these two callbacks and how they differ.

> > "BeforeTraverseReal" and "MaybeClearPurpleReal" don't really describe what
> > the functions do, and should probably be more similar given how similar the
> > functions are.  Could these be changed to something like
> > "MustBeLiveBeforeTraverse" and "MustBeLive"?  "Always" instead of "MustBe"
> > maybe?
> No. Since cycle collector just calls some helpers (which may or may not be
> implemented)

How about something like CanSkipTraverse and CanSkipTraverseInCC for MaybeClearPurple and BeforeTraverse?  If one of those returns true, then we know that the CC doesn't have to visit it, but if it returns false, then we're not saying it is alive or anything, just that the CC has to visit it.  That's a pretty weak statement, and so it is okay for the callbacks to not do much if they don't want to.

> > Can you define this macro as NS_IMPL_CYCLE_COLLECTION_CLASS(_class)?  So it
> > is more obvious you are just doing the same thing.
> Don't understand this
Yeah, I clicked in the wrong place so splinter didn't give useful output.  Sorry.  What I meant was, the definition of NS_IMPL_CYCLE_COLLECTION_BEFORE_TRAVERSE_CLASS is the same as the definition of NS_IMPL_CYCLE_COLLECTION_CLASS, so couldn't the former be defined in terms of the latter?

> > This is unused.  Maybe it makes sense to put it into a later patch?  I'm not
> > sure how exactly it is used.  It overlaps a lot with unmarkPurple.
> unmarkPurple doesn't set the buffer to null

Can you define it in terms of unmarkPurple?  Like, set the buffer to null, then call unmarkPurple?  (I'll look over your full patch to see how RemovePurple is used.)

Comment 10

[Andrew McCreight [:mccr8]]

In fact, because the callbacks are in the class nsXPCOMCycleCollectionParticipant, the Traverse part can probably be omitted, and they could be called CanSkip and CanSkipInCC or something?  Really, we're doing more than skipping their traversal, we're skipping them altogether.

Comment 11

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

Attachment: renaming stuff

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 587361 [details] [diff] [review]
renaming stuff

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

The names look much better!

I think the name of nsCycleCollector::RemoveSkippable should indicate somehow that we're removing purple buffer entries.  Maybe just ForgetSkippable, as "suspect" and "forget" are the terms we use in the CC for adding and removing things from the purple buffer, or RemoveSkippablePurple, though that is a bit long.

Likewise, "nsCycleCollector_removeSkippable" should become something like "nsCycleCollector_forgetSkippable".  I think "forget" is probably best for this function, because forget is used already for the external interface, in NS_CycleCollectorForget2, but purple could be okay, too.

RemoveSkippable is fine for the purple buffer because it is implied that you are removing purple buffer entries.

The clear purple callback stuff (mClearPurpleCB, CC_ClearPurpleCallback, nsCycleCollector_setClearPurpleCallback) should be renamed to match whatever nsCycleCollector::RemoveSkippable() ends up being called.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +181,5 @@
>      bool CheckForRightISupports(nsISupports *s);
> +
> +    bool CanSkipInCC(void *p)
> +    {
> +        if (mNeedsSkip) {

while I'm here, this whole method could be done as
  return mNeedsSkip && CanSkipInCCReal(p)
and likewise for CanSkip.

Maybe mNeedsSkip could be mMightSkip?

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 587361 [details] [diff] [review]
renaming stuff

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

The macro names should probably be updated, too.

I still haven't really looked over RemovePurple, but as that isn't being used anywhere the name should be easy to fix for this patch.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +269,5 @@
>  
>  #define NS_CYCLE_COLLECTION_UPCAST(obj, clazz)                                 \
>    NS_CYCLE_COLLECTION_CLASSNAME(clazz)::Upcast(obj)
>  
> +#define NS_IMPL_CYCLE_COLLECTION_CLEAR_IN_CC_BEGIN(_class)                     \

maybe this could be NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN?

@@ +278,5 @@
> +    NS_ASSERTION(CheckForRightISupports(s),                                    \
> +                 "not the nsISupports pointer we expect");                     \
> +    _class *tmp = Downcast(s);
> +
> +#define NS_IMPL_CYCLE_COLLECTION_CLEAR_IN_CC_END                               \

(this would need to be updated, of course)

@@ +282,5 @@
> +#define NS_IMPL_CYCLE_COLLECTION_CLEAR_IN_CC_END                               \
> +    return false;                                                              \
> +  }
> +
> +#define NS_IMPL_CYCLE_COLLECTION_CLEAR_BEGIN(_class)                           \

NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN?

@@ +291,5 @@
> +    NS_ASSERTION(CheckForRightISupports(s),                                    \
> +                 "not the nsISupports pointer we expect");                     \
> +    _class *tmp = Downcast(s);
> +
> +#define NS_IMPL_CYCLE_COLLECTION_CLEAR_END                                     \

(would need to be updated, too)

@@ +602,5 @@
>    NS_IMETHOD_(void) Trace(void *p, TraceCallback cb, void *closure);           \
>  };                                                                             \
>  NS_CYCLE_COLLECTION_PARTICIPANT_INSTANCE
>  
> +#define NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_WITH_CLEAR_CLASS_AMBIGUOUS(_class, _base)  \

NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS_AMBIGUOUS or something like that?

@@ +616,5 @@
> +  NS_IMETHOD_(bool) CanSkipReal(void *p);                                                 \
> +};                                                                                        \
> +NS_CYCLE_COLLECTION_PARTICIPANT_INSTANCE
> +
> +#define NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_WITH_CLEAR_CLASS(_class)  \

maybe something like NS_DECL_CYCLE_COLLECTION_SKIPPABLE_SCRIPT_HOLDER_CLASS

Comment 14

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

Attachment: patch

And this even runs :)

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 587414 [details] [diff] [review]
patch

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

Looks good.  It is getting there.

This is my review for all but the changes in nsISupportsImpl.h (RemovePurple).  I need to finish re-reading the existing code for unpurpling first to figure that out.  If you are in a hurry for the rest of this patch, you can split that off into a separate bug, but I should be able to get to that this afternoon.

::: xpcom/base/nsCycleCollector.cpp
@@ +1939,5 @@
> +                if (e->mObject) {
> +                    nsISupports* o = canonicalize(e->mObject);
> +                    nsXPCOMCycleCollectionParticipant* cp;
> +                    ToParticipant(o, &cp);
> +                    freeEntry = cp->CanSkip(o);

Did you not like my suggestion to do
if (!cp->CanSkip(o))
  continue;
here, and then remove freeEntry entirely?  That's okay if you don't like it, either way is a little gross, I just wanted to make sure you considered it here.

@@ +1944,5 @@
> +                    if (freeEntry) {
> +                        cp->UnmarkPurple(o);
> +                    }
> +                }
> +                if (freeEntry) {

For this block of code, I think you can call Remove(e) instead.  While you are at it, if you could replace the same chunk of code in SelectPointers with Remove, I'd appreciate it. After double checking that they are actually all the same code, of course.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +165,5 @@
>  {
>  public:
> +    nsXPCOMCycleCollectionParticipant() : mMightSkip(false) {}
> +    nsXPCOMCycleCollectionParticipant(bool aSkip)
> +    : mMightSkip(aSkip) {}

Should this line (: mMightSkip etc) be indented?  I'm not sure.

@@ +181,5 @@
>      bool CheckForRightISupports(nsISupports *s);
> +
> +    bool CanSkipInCC(void *p)
> +    {
> +        return mMightSkip ? CanSkipInCCReal(p) : false;

indented too much

@@ +187,5 @@
> +
> +    bool CanSkip(void *p)
> +    {
> +      return mMightSkip ? CanSkipReal(p) : false;
> +    }

blank line before protected, maybe?

@@ +189,5 @@
> +    {
> +      return mMightSkip ? CanSkipReal(p) : false;
> +    }
> +protected:
> +    NS_IMETHOD_(bool) CanSkipInCCReal(void *p) { return false; }

May want to add MOZ_ASSERT(false) in CanSkipInCCReal and CanSkipReal, because we should never hit the virtual methods unless we changed them.

Comment 16

[Andrew McCreight [:mccr8]]

Also, if you could add brief comments to RemoveSkippable, CanSkipInCC, and CanSkip describing what they do/are for that would be good.

Comment 17

[Andrew McCreight [:mccr8]]

Why do you need RemovePurple instead of NS_CycleCollectorForget2?  I'm concerned that marking entries as freed but not adding them to the free list in the purple buffer will cause fragmentation.  NS_CycleCollectorForget2 is called inside of nsCycleCollectingAutoRefCnt::incr and decr so it would seem to me that it could be called fairly freely.

Comment 18

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

(In reply to Andrew McCreight [:mccr8] from comment #15) 
> This is my review for all but the changes in nsISupportsImpl.h
> (RemovePurple).  I need to finish re-reading the existing code for
> unpurpling first to figure that out.  If you are in a hurry for the rest of
> this patch, you can split that off into a separate bug, but I should be able
> to get to that this afternoon.
Nothing of this is really useful without RemovePurple :)



> @@ +1939,5 @@
> > +                if (e->mObject) {
> > +                    nsISupports* o = canonicalize(e->mObject);
> > +                    nsXPCOMCycleCollectionParticipant* cp;
> > +                    ToParticipant(o, &cp);
> > +                    freeEntry = cp->CanSkip(o);
> 
> Did you not like my suggestion to do
> if (!cp->CanSkip(o))
>   continue;
> here, and then remove freeEntry entirely?  That's okay if you don't like it,
> either way is a little gross, I just wanted to make sure you considered it
> here.
Well, I need to handle the !e->mObject case, but sure
I could change the code to

if (e->mObject) {
  ...
  if (!cp->CanSkip(o)) {
    continue;
  }
}




> 
> @@ +1944,5 @@
> > +                    if (freeEntry) {
> > +                        cp->UnmarkPurple(o);
> > +                    }
> > +                }
> > +                if (freeEntry) {
> 
> For this block of code, I think you can call Remove(e) instead.  While you
> are at it, if you could replace the same chunk of code in SelectPointers
> with Remove, I'd appreciate it. After double checking that they are actually
> all the same code, of course.
Ah, because of SelectPointers I didn't notice that there is Remove()


> > +    nsXPCOMCycleCollectionParticipant() : mMightSkip(false) {}
> > +    nsXPCOMCycleCollectionParticipant(bool aSkip)
> > +    : mMightSkip(aSkip) {}
> 
> Should this line (: mMightSkip etc) be indented?  I'm not sure.
well, I could move : to previous line and keep mMightSkip where it is now, or something :)


> @@ +181,5 @@
> >      bool CheckForRightISupports(nsISupports *s);
> > +
> > +    bool CanSkipInCC(void *p)
> > +    {
> > +        return mMightSkip ? CanSkipInCCReal(p) : false;
> 
> indented too much
Bah, style in CC code is so messy. Some is using 4 space indentation, some is not.
Everything should be using 2 spaces.


> 
> May want to add MOZ_ASSERT(false) in CanSkipInCCReal and CanSkipReal,
> because we should never hit the virtual methods unless we changed them.
Ok.

Comment 19

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

(In reply to Andrew McCreight [:mccr8] from comment #17)
> Why do you need RemovePurple instead of NS_CycleCollectorForget2?
I need to make mObject null.

> I'm
> concerned that marking entries as freed but not adding them to the free list
> in the purple buffer will cause fragmentation.
RemovePurple should in practice be called only when RemoveSkippable/SelectPointers is being
called, so the entries with null objects will be removed immediately from purple buffer.
(And if that doesn't happen immediately, it happens when RemoveSkippable/SelectPointers is
 run next time. But this case shouldn't actually happen ever with my patches.)

>  NS_CycleCollectorForget2 is
> called inside of nsCycleCollectingAutoRefCnt::incr and decr so it would seem
> to me that it could be called fairly freely.
But it doesn't set mObject to null.

Comment 20

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

(In reply to Andrew McCreight [:mccr8] from comment #15)
> > +    bool CanSkipInCC(void *p)
> > +    {
> > +        return mMightSkip ? CanSkipInCCReal(p) : false;
> 
> indented too much
> 
Actually no. The class uses 4 spaces.

Comment 21

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

(In reply to Olli Pettay [:smaug] from comment #19) 
> >  NS_CycleCollectorForget2 is
> > called inside of nsCycleCollectingAutoRefCnt::incr and decr so it would seem
> > to me that it could be called fairly freely.
> But it doesn't set mObject to null.
Ah, but NS_CycleCollectorForget2 calls Remove. That should work after all.

Comment 22

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

But something is needed to nsCycleCollectingAutoRefCnt so that purplebufferentry can
be accessed, so I keep RemovePurple but make it use NS_CycleCollectorForget2.

Comment 23

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

and I don't use MOZ_ASSERT since I would need to bring in more headers to each CCable class.
And, the assert doesn't need to be fatal. NS_ASSERTION is just fine.

And I need RemovePurple() as it is. NS_CycleCollectorForget2 doesn't work when mScanInProgress.

Comment 24

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

Attachment: patch

Comment 25

[Andrew McCreight [:mccr8]]

Comment on attachment 587654 [details] [diff] [review]
patch

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

Everything but RemovePurple looks good to me.  I have a few minor comments on the other things, but nothing big.

::: xpcom/base/nsCycleCollector.cpp
@@ +868,5 @@
>      void SelectPointers(GCGraphBuilder &builder);
>  
> +    // RemoveSkippable iterates nsPurpleBufferEntry objects
> +    // and removes them from PurpleBuffer if nsPurpleBufferEntry::mObject
> +    // is null or if object's nsXPCOMCycleCollectionParticipant::CanSkip()

maybe "RemoveSkippable removes entries from the purple buffer" instead of "RemoveSkippable iterates nsPurpleBufferEntry objects and removes them from PurpleBuffer".

::: xpcom/base/nsCycleCollector.h
@@ +67,5 @@
> +typedef void (*CC_BeforeUnlinkCallback)(void);
> +void nsCycleCollector_setBeforeUnlinkCallback(CC_BeforeUnlinkCallback aCB);
> +typedef void (*CC_ForgetSkippableCallback)(void);
> +void nsCycleCollector_setForgetSkippableCallback(CC_ForgetSkippableCallback aCB);
> +void nsCycleCollector_forgetSkippable();

Maybe add a space after setForgetSkippableCallback?  It feels slightly odd for forgetSkippable to be mashed in with these very boiler plate kind of functions.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +189,5 @@
> +    // If CanSkip returns true, p is removed from purple buffer
> +    // during nsCycleCollector_forgetSkippable() call.
> +    bool CanSkip(void *p)
> +    {
> +      return mMightSkip ? CanSkipReal(p) : false;

okay, well, then this isn't indented enough. ;)  I don't care which way, but it should be consistent.

Comment 26

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] from comment #23)
> And I need RemovePurple() as it is. NS_CycleCollectorForget2 doesn't work
> when mScanInProgress.
Okay, now I see.  What you need is a version of NS_CycleCollectorForget2 that works while the CC is running. Do you need a function that works only during the CC, or should it also work when the CC isn't running?

As far as I can tell, all that it really does is update the purple buffer free list.  I guess all the guards on Forget2 are because it can be caused by addrefs and decrefs, and we don't want it somehow happening in the middle of a function that is manipulating the free list.

Can we just make a version of NS_CycleCollectorForget2 that doesn't do those checks and use that?  It looks to me like it would be okay to call any time except in the middle of updating the free list, and we're not calling any outside code during that time.

Or, if you really only need to call it during the CC, then you could keep going with the current nulling out method you are using.

But in either case, I'd prefer it if you created some kind of variant of NS_CycleCollectorForget2 (and Forget2), so we can have the various side checks and DEBUG_CC gunk that regular Forget has.

Comment 27

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

> > +    // during nsCycleCollector_forgetSkippable() call.
> > +    bool CanSkip(void *p)
> > +    {
> > +      return mMightSkip ? CanSkipReal(p) : false;
> 
> okay, well, then this isn't indented enough. ;)  I don't care which way, but
> it should be consistent.
Bah, it was right and then I changed it before noticing I was making it inconsistent with rest of the
file.

Comment 28

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

(In reply to Andrew McCreight [:mccr8] from comment #26)
> (In reply to Olli Pettay [:smaug] from comment #23)
> > And I need RemovePurple() as it is. NS_CycleCollectorForget2 doesn't work
> > when mScanInProgress.
> Okay, now I see.  What you need is a version of NS_CycleCollectorForget2
> that works while the CC is running. Do you need a function that works only
> during the CC, or should it also work when the CC isn't running?

I need to be able to clear nsPurpleBufferEntry::mObject at any point.


> As far as I can tell, all that it really does is update the purple buffer
> free list.  I guess all the guards on Forget2 are because it can be caused
> by addrefs and decrefs, and we don't want it somehow happening in the middle
> of a function that is manipulating the free list.

I don't see anything wrong with just setting mObject to null.


> Can we just make a version of NS_CycleCollectorForget2 that doesn't do those
> checks and use that?
And make a strong warning that only RemovePurple should use it?

> But in either case, I'd prefer it if you created some kind of variant of
> NS_CycleCollectorForget2 (and Forget2), so we can have the various side
> checks and DEBUG_CC gunk that regular Forget has.
Ok, but I will call it something very different than NS_CycleCollectorForget2, since I want
to make it clear that it should be called only from RemovePurple. Also, it slows down
RemovePurple a _tiny_ bit, since it goes via one reference.

Comment 29

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

Adding anything like NS_CycleCollector* is just painful. Need to add all sort of glue code :/

Comment 30

[Andrew McCreight [:mccr8]]

I think I've come around to having RemovePurple in place.  I'd like to see:
- add the DEBUG_CC forget logging stuff
- RemovePurple should call unmarkPurple first, then do the nulling, instead of just copying the code over.
- There needs to be a comment about what it does and what it is for.
- I don't really like the name, but I can't think of a better one.  The thing is, it doesn't fully remove it.  Well, it removes the element, but doesn't add the thing to the free list.

Comment 31

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

Attachment: This works with and without DEBUG_CC

I added IsBlack and fixed DEBUG_CC handling.
(Note, if you have both DEBUG and DEBUG_CC defined, compiling nsXPConnect fails, but that has nothing to do with this bug.)

Comment 32

[Andrew McCreight [:mccr8]]

Comment on attachment 588504 [details] [diff] [review]
This works with and without DEBUG_CC

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

This is mostly just comments-on-comments.

IsBlack needs a better name that is more similar to the other CanSkip ones.  CanSkipConst or maybe something else.

r=me with these comments addressed.

::: xpcom/base/nsCycleCollector.cpp
@@ +868,5 @@
>      void SelectPointers(GCGraphBuilder &builder);
>  
> +    // RemoveSkippable removes entries from the purple buffer
> +    // if nsPurpleBufferEntry::mObject is null or
> +    // if object's nsXPCOMCycleCollectionParticipant::CanSkip() returns true.

"if object's" or something could probably be moved to the previous line.  And maybe it should be "if the object's".

@@ +1152,5 @@
>      FILE *mPtrLog;
>  
>      void Allocated(void *n, size_t sz);
>      void Freed(void *n);
> +    void LogPurpleRemoval(void* aObject);

Please put a blank line before and after the declaration of LogPurpleRemoval, to separate out the three types of DEBUG_CC-related declarations.

@@ +1787,5 @@
>      
>      nsXPCOMCycleCollectionParticipant *cp;
>      ToParticipant(child, &cp);
>      if (cp) {
> +        if (cp->IsBlack(child)) {

Instead of doing this return, you could change the prior line to if (cp && !cp->IsBlack(child)) {

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +178,5 @@
>      NS_IMETHOD_(void) UnmarkPurple(nsISupports *p);
>  
>      bool CheckForRightISupports(nsISupports *s);
> +
> +    // If CanSkipInCC returns true, p is skipped when

The first line of the comment is a little short, so you could move some comments up there.

@@ +179,5 @@
>  
>      bool CheckForRightISupports(nsISupports *s);
> +
> +    // If CanSkipInCC returns true, p is skipped when
> +    // moving nodes from purple buffer to cycle collector's graph.

"purple buffer" to "the purple buffer" maybe, and likewise "the cycle collector's graph".
Maybe instead of "moving nodes from purple buffer to cycle collector's graph" this could be "selecting roots for the cycle collector graph"?

@@ +180,5 @@
>      bool CheckForRightISupports(nsISupports *s);
> +
> +    // If CanSkipInCC returns true, p is skipped when
> +    // moving nodes from purple buffer to cycle collector's graph.
> +    // Note, calling CanSkipInCC may remove objects from purple buffer!

"the purple buffer".  Also, you should document that it shouldn't remove the current object from the purple buffer, maybe just by saying "may remove other objects", but perhaps that is too subtle.

@@ +187,5 @@
> +        return mMightSkip ? CanSkipInCCReal(p) : false;
> +    }
> +
> +    // If CanSkip returns true, p is removed from purple buffer
> +    // during nsCycleCollector_forgetSkippable() call.

"during a call to nsCycleCollector_forgetSkippable()", maybe?

@@ +188,5 @@
> +    }
> +
> +    // If CanSkip returns true, p is removed from purple buffer
> +    // during nsCycleCollector_forgetSkippable() call.
> +    // Note, calling CanSkip may remove objects from purple buffer!

"the purple buffer" instead of "purple buffer" in two places.
Maybe CanSkip should be before CanSkipInCC in this file?  Also, same thing above about somehow implying it shouldn't remove this object from the purple buffer.

@@ +194,5 @@
> +    {
> +        return mMightSkip ? CanSkipReal(p) : false;
> +    }
> +
> +    // If IsBlack returns true, p is not added to the graph.

This needs a comment saying that this is called while the CC is running, so it shouldn't touch the purple buffer, or affect the state of any object.

@@ +199,5 @@
> +    bool IsBlack(void *p)
> +    {
> +        return mMightSkip ? IsBlackReal(p) : false;
> +    }
> +protected:

Change the order of the "Real" declarations if you go with my suggestion to change the CanSkip order above.

@@ +642,5 @@
> +public:                                                                                   \
> +  NS_CYCLE_COLLECTION_INNERCLASS () : nsXPCOMCycleCollectionParticipant(true) {}          \
> +  NS_DECL_CYCLE_COLLECTION_CLASS_BODY(_class, _base)                                      \
> +  NS_IMETHOD_(void) Trace(void *p, TraceCallback cb, void *closure);                      \
> +protected:                                                                                \

Change the order of the "Real" declarations if you go with my suggestion to change the CanSkip order above.

::: xpcom/glue/nsISupportsImpl.h
@@ +225,5 @@
>      nsrefcnt refcount = NS_CCAR_TAGGED_TO_PURPLE_ENTRY(mTagged)->mRefCnt;
>      mTagged = NS_CCAR_REFCNT_TO_TAGGED(refcount);
>    }
>  
> +  void RemovePurple()

Maybe add a comment saying that the entry will get added to the free list later.

Comment 33

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

Attachment: patch

I'll upload this to tryserver before pushing to m-c

Comment 34

[Andrew McCreight [:mccr8]]

Comment on attachment 588634 [details] [diff] [review]
patch

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

Looks good!  A few more minor comments.

::: xpcom/base/nsCycleCollector.cpp
@@ +2751,5 @@
>  
>      if (mParams.mLogPointers) {
>          if (!mPtrLog)
>              mPtrLog = fopen("pointer_log", "w");
> +        fprintf(mPtrLog, "F %p\n", static_cast<void*>(aObject));

I don't think you need this cast from void* to void*.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +179,5 @@
>  
>      bool CheckForRightISupports(nsISupports *s);
> +
> +    // If CanSkip returns true, p is removed from the purple buffer during
> +    // a call to nsCycleCollector_forgetSkippable() call.

second "call" can be removed.

@@ +188,5 @@
> +    }
> +
> +    // If CanSkipInCC returns true, p is skipped when selecting roots for the
> +    // cycle collector graph.
> +    // Note, calling CanSkipInCC may remove other objects from purple buffer!

"from the purple buffer"

Comment 35

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

Attachment: patch

Comment 36

[Andrew McCreight [:mccr8]]

Comment on attachment 588637 [details] [diff] [review]
patch

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

I guess I don't really need to r+ all of these minor revisions, but I will anyways.

The one comment still has "call" in it twice, but otherwise this looks good.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +179,5 @@
>  
>      bool CheckForRightISupports(nsISupports *s);
> +
> +    // If CanSkip returns true, p is removed from the purple buffer during
> +    // a call to nsCycleCollector_forgetSkippable() call.

This comment still has call twice.  It should be "a call to nsCycleCollector_forgetSkippable()." not "a call to nsCycleCollector_forgetSkippable() call."

Comment 37

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

Attachment: patch

Comment 38

[Andrew McCreight [:mccr8]]

Comment on attachment 588639 [details] [diff] [review]
patch

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

Hooray!

Comment 39

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

Finally. You're quite a nitpicker ;)

Comment 40

[Andrew McCreight [:mccr8]]

Yeah, sorry.  You are creating a whole new framework here so I wanted to make sure it was really polished.

Comment 41

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

Yup, I understand. And good that someone reviews my bad English :)

Comment 42

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

https://hg.mozilla.org/mozilla-central/rev/49afabda6701