Comment on attachment 711269 [details] [diff] [review] Patch Review of attachment 711269 [details] [diff] [review]: ----------------------------------------------------------------- Looks good over all, thanks for doing this! r- for the FixGrayBits issue. ::: xpcom/base/nsCycleCollector.cpp @@ +1027,5 @@ > + bool mCollected; > + bool mMergeCompartments; > + > +public: > + NS_IMETHOD Run(); While you are touching this, you might as well move Run() after the constructor to make it more standard. Unless this is how people define Runnables... @@ +1029,5 @@ > + > +public: > + NS_IMETHOD Run(); > + > + nsCycleCollectorRunner(nsCycleCollector *collector) aCollector @@ +1097,5 @@ > > nsPurpleBuffer mPurpleBuf; > > + // Holds a reference. > + nsCycleCollectorRunner* mRunner; Why are mRunner and mThread raw pointers rather than RefPtrs? @@ +2600,2 @@ > #endif > + mRunner(nullptr), Probably just move mRunner and mThread before mPurpleBuf in both the declarations and here in the constructor to avoid grossness, and so you are instantiating them in the same order everywhere. @@ +3253,5 @@ > + nsICycleCollectorListener *aListener) > +{ > + MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!"); > + > + // On a WantAllTraces CC, force a synchronous global GC to prevent There's a basic problem with this patch is that the FixGrayBits argument needs to change if it is shutdown or not. At shutdown, it should always be true, as it is in the Collect function. If it isn't shutdown, then you have to do the WantAllTraces dance that is here. The additional twist is that you can't call FixGrayBits on the CC thread. So you are going to need some refactoring in how things are initialized. Have you tried testing this with nsCycleCollector_startup(RunOnThread)? I think that would fire off various assertions with the current setup. You also want to check that it doesn't become massively slower. A few ms one way or the other don't matter, you mostly want to make sure it isn't getting like 100ms longer. @@ +3381,5 @@ > if (!aListener && sCollector && sCollector->mParams.mLogAll) { > listener = new nsCycleCollectorLogger(); > } > > + if (sCollector->mRunner) { Please turn this if-then-else block into a method Collect, and rename the existing nsCycleCollector::Collect to something like MainThreadCollect that is consistent with CCThreadingModel. ::: xpcom/base/nsCycleCollector.h @@ +27,5 @@ > uint32_t mFreedGCed; > }; > > +enum CCThreadingModel { > + RunOnThread, These enums should have something about the CC in them, like CCRunOnThread or something. OnThread/OffThread isn't too great either. OnMainThread/OffMainThread? Is "main thread" what you'd call the primary thread in a worker? CCSingleThreaded/CCExtraThread or something? (Describing it as "multithreaded" is probably confusing.)

Comment on attachment 720884 [details] [diff] [review] Part 1: Refactor a bunch of junk Review of attachment 720884 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/nsCycleCollector.cpp @@ +867,5 @@ > } > > +class nsCycleCollector; > + > +class nsCycleCollectorRunner : public nsRunnable I'm going to assume you didn't change anything substantial in the class declaration. @@ +936,3 @@ > { > + friend class GCGraphBuilder; > +private: I think this |private:| isn't really needed. @@ +945,5 @@ > nsCycleCollectionJSRuntime *mJSRuntime; > > GCGraph mGraph; > > + nsCycleCollectorRunner* mRunner; nit: * to the right I asked this before, but I don't remember if you answered it out of band or something... why is mRunner a raw pointer? Because you can't use an nsRefPtr for off-main-thread stuff? Please document here that this is a strong reference to the runner. @@ +972,5 @@ > > + inline nsCycleCollectionJSRuntime* > + JSRuntime() const > + { > + return mJSRuntime; nit: indent 4 spaces @@ +977,5 @@ > + } > + > + void SetBeforeUnlinkCallback(CC_BeforeUnlinkCallback aBeforeUnlinkCB) > + { > + mBeforeUnlinkCB = aBeforeUnlinkCB; nit: indent 4 spaces @@ +982,5 @@ > + } > + > + void SetForgetSkippableCallback(CC_ForgetSkippableCallback aForgetSkippableCB) > + { > + mForgetSkippableCB = aForgetSkippableCB; nit: indent 4 spaces @@ +2698,5 @@ > +nsCycleCollector::Collect(bool aMergeCompartments, > + nsCycleCollectorResults *aResults, > + nsICycleCollectorListener *aListener) > +{ > + mRunner->Collect(aMergeCompartments, aResults, aListener); To preserve existing behavior, this should be if (mRunner) { mRunner->Collect(aMergeCompartments, aResults, aListener); } else { MainThreadCollect(aMergeCompartments, aResults, 1, aListener); } Though arguably if somebody is actually hitting this case, it might be better to just crash them...

(In reply to Andrew McCreight [:mccr8] from comment #3) > To preserve existing behavior, this should be > if (mRunner) { > mRunner->Collect(aMergeCompartments, aResults, aListener); > } else { > MainThreadCollect(aMergeCompartments, aResults, 1, aListener); > } > Though arguably if somebody is actually hitting this case, it might be > better to just crash them... You may be missing the point. One of the ideas here is that there's always an mRunner (except for shutdown collections).

> You may be missing the point. One of the ideas here is that there's always an mRunner (except for shutdown collections). Yes, I don't understand what you mean here. Is your patch expected to change the behavior of something? Is the idea that with your patch, if we never call nsCycleCollectorRunner::Init(), then we just end up running the runner on the main thread? Or something else?

I'm not really sure how to do performant release mode assertions in this brave new world, but this is better than nothing.

Over IRC, khuey said the point of part 1 is that we'll always have a runner, even if we have no other thread, except at shutdown. You should add a comment to that effect somewhere in there, on the runner class, I think, as it isn't super obvious.

Comment on attachment 721549 [details] [diff] [review] Part 2: Wean GCGraphBuilder off of a global CC reference Review of attachment 721549 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the delay on these reviews.

Comment on attachment 721550 [details] [diff] [review] Part 3: Move the CC into TLS Review of attachment 721550 [details] [diff] [review]: ----------------------------------------------------------------- Should you explicitly #include the header for ThreadLocal? r- because I'm not sure what nsCycleCollector::init() is supposed to be. Looks good otherwise, modulo a few nits. ::: xpcom/base/nsCycleCollector.cpp @@ +1160,5 @@ > //////////////////////////////////////////////////////////////////////// > // The static collector object > //////////////////////////////////////////////////////////////////////// > > +static mozilla::ThreadLocal<nsCycleCollector*> sCollector; Is this supposed to be static? The other usages of this in the tree look like they aren't. It would be good to get the opinion of somebody who knows about this. Then maybe it should be gCollector? I don't know how that goes precisely. @@ +2840,5 @@ > > void > nsCycleCollector_registerJSRuntime(nsCycleCollectionJSRuntime *rt) > { > + nsCycleCollector* collector = sCollector.get(); nit: * to the right here and in the rest of this patch. You could also use the form if (nsCycleCollector *collector = sCollector.get()) { ... } here but I'm not particularly wedded to it so feel free to leave it as is. ;) @@ +2887,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread(), "Wrong thread!"); > + MOZ_ASSERT(!sCollector.initialized(), "Called twice!?"); > + > + return sCollector.init(); What is "init"? You never define it anywhere as far as I can see. @@ +2895,5 @@ > nsCycleCollector_startup() > { > + MOZ_ASSERT(sCollector.initialized(), > + "Forgot to call nsCycleCollector_init!"); > + MOZ_ASSERT(!sCollector.get(), "Forgot to call nsCycleCollector_shutdown?"); I think this should be a dynamic check. If we have an existing collector, it seems to me that we should fail, rather than just silently replace the existing one. Though maybe assert, too... @@ +2899,5 @@ > + MOZ_ASSERT(!sCollector.get(), "Forgot to call nsCycleCollector_shutdown?"); > + > + nsCycleCollector* collector = new nsCycleCollector(); > + > + nsresult rv = collector->Init(); If Init() is to be called from the _threadStartup function and not _init, maybe it should be called ThreadStartup or something and not Init... @@ +2902,5 @@ > + > + nsresult rv = collector->Init(); > + if (NS_SUCCEEDED(rv)) { > + sCollector.set(collector); > + } You should delete the collector on failure. ::: xpcom/base/nsCycleCollector.h @@ +24,5 @@ > uint32_t mFreedRefCounted; > uint32_t mFreedGCed; > }; > > +bool nsCycleCollector_init(); Please add some comments about what _init() and _startup() are for, and maybe even rename them so it is clear what they do. Maybe _init should be _globalInit and _startup() should be _threadStartup()?

Comment on attachment 721550 [details] [diff] [review] Part 3: Move the CC into TLS Review of attachment 721550 [details] [diff] [review]: ----------------------------------------------------------------- Oops, right, .init is the TLS init thing. It would still be nice to have better names for _init() _startup() and Init(). :)

Comment on attachment 721886 [details] [diff] [review] Part 5: Offer the option of using another thread or not Review of attachment 721886 [details] [diff] [review]: ----------------------------------------------------------------- Maybe MainThreadCollect() should be ShutdownCollect()? I wasn't sure where you were going with it when I reviewed part 1. I still need to mull over your assertion changes, which is why I haven't finished part 4 yet.

Comment on attachment 721551 [details] [diff] [review] Part 4: Add back some threadsafety assertions I have some reservations of paring back these assertions too heavily, and I discussed this with khuey and IRC. He has an idea for basically asserting for things that are not on the main thread or a worker thread, and he said the main use case here is to kill the browser when addons are doing bad things. So I'm clearing the review flag here. Kyle, if you'd prefer, I'd be okay with leaving all the assertions in place as is, and dealing with fixing them for workers a followup bug. That will make actually using the CC in a worker thread not work (but maybe you could still do mainthread-only CC?), but you could get this big chunk of work landed. Up to you.

Comment on attachment 727250 [details] [diff] [review] Part 4: Add back some threadsafety assertions Review of attachment 727250 [details] [diff] [review]: ----------------------------------------------------------------- I'd be a little surprised if these MOZ_CRASHes you added to the CC entry points don't crash (weird things happen in shutdown and in XPCShell), but if it passes TBPL, sounds fine to me. (khuey told me on IRC that the purpose of these checks is to ensure we never try to call the CC on a thread without a CC.) Thanks for doing this, the CC thread stuff is easier to understand with your patches. ::: xpcom/base/nsCycleCollector.cpp @@ +2567,5 @@ > return true; > } > > +void > +nsCycleCollector::CheckThreadSafety() Please move this definition before suspect2. @@ +2570,5 @@ > +void > +nsCycleCollector::CheckThreadSafety() > +{ > +#ifdef DEBUG > + MOZ_ASSERT(mThread == PR_GetCurrentThread()); I'd be mildly surprised if this worked, as I said in IRC, but it would be cool if it did! If it doesn't, just move the check after the mScanInProgress check in Suspect/Forget and then we just won't do the check when we're on the CC thread. @@ +2873,5 @@ > NS_CycleCollectorSuspect2(void *n, nsCycleCollectionParticipant *cp) > { > nsCycleCollector *collector = sCollector.get(); > > + if (!collector) nit: brace this, as you are bracing the rest. @@ +2884,5 @@ > NS_CycleCollectorForget2(nsPurpleBufferEntry *e) > { > nsCycleCollector *collector = sCollector.get(); > > + if (!collector) nit: brace this, as you are bracing the rest.