901630 - Remove support for the CC thread

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

We're not using it, let's get rid of it.

Comment 1

[Luke Wagner [:luke]]

If you're enjoying the reaping, you could remove JS_ClearRuntimeThread/JS_SetRuntimeThread and the CycleCollectedJSRuntime methods that call the interface methods they implement.  I can review :)

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This removes the consumers of JS_SetRuntimeThread.  I don't want to remove the API in this bug.

Comment 3

[Andrew McCreight [:mccr8]]

Given that the GC is going to dance all over memory every 10 seconds, and our CC graphs are so much smaller than they were before, I can't imagine this extra complexity (in both the cycle collector and the JS engine) is buying us much nowadays, so it seems like a good thing to remove.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Given that the GC is going to dance all over memory every 10 seconds, and
> our CC graphs are so much smaller than they were before, I can't imagine
> this extra complexity (in both the cycle collector and the JS engine) is
> buying us much nowadays

This is definitely possible. You guys have made a ton of optimizations to the CC that could very well have made this optimization irrelevant. (One quibble: the JS heap isn't nearly as big a space as all the objects reachable via the Traverse hooks, right? It bet it depends a lot on the way the browser is being used).

> so it seems like a good thing to remove.

This code in particular is not something that I care a great deal about (and this will be the last time I speak in its defense) but in general I think we shouldn't be changing perf-sensitive code without even attempting to measure.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 785942 [details] [diff] [review]
Patch

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

Thanks for fixing this!

Please file a bug to remove the JS API functions you removed the use of.

Please remove this reference to nsCycleCollectorRunner and get a rs from a Cleopatra person to make sure it won't do anything weird:
  http://mxr.mozilla.org/mozilla-central/source/browser/devtools/profiler/cleopatra/js/parserWorker.js#1421

::: xpcom/base/nsCycleCollector.cpp
@@ +108,5 @@
>  #include "nsBaseHashtable.h"
>  #include "nsHashKeys.h"
>  #include "nsDeque.h"
>  #include "nsCycleCollector.h"
>  #include "nsThreadUtils.h"

While you are here, please fix this double-include of nsThreadUtils.h.

@@ +109,5 @@
>  #include "nsHashKeys.h"
>  #include "nsDeque.h"
>  #include "nsCycleCollector.h"
>  #include "nsThreadUtils.h"
>  #include "prenv.h"

Also, maybe you can remove some of the #ifdef WIN32 or #ifdef XP_WIN includes in this file?  I don't see any other blocks in this file under those guards after your patch.

@@ +110,5 @@
>  #include "nsDeque.h"
>  #include "nsCycleCollector.h"
>  #include "nsThreadUtils.h"
>  #include "prenv.h"
>  #include "prprf.h"

Remove this whole block from nsCycleCollector.cpp:
#if defined(XP_WIN)
// Defined in nsThreadManager.cpp.
extern DWORD gTLSThreadIDIndex;
#elif defined(NS_TLS)
// Defined in nsThreadManager.cpp.
extern NS_TLS mozilla::threads::ID gTLSThreadID;
#else
PRThread* gCycleCollectorThread = nullptr;
#endif

@@ +918,5 @@
>      TimeStamp mCollectionStart;
>  
>      CycleCollectedJSRuntime *mJSRuntime;
> +    nsICycleCollectorListener *mListener;
> +    bool mCollected;

mListener and mCollected don't need to be fields

@@ +980,4 @@
>      ~nsCycleCollector();
>  
>      nsresult Init();
>      void ShutdownThreads();

Remove the declaration of the undefined methods Init and ShutdownThreads.

@@ -1029,5 @@
>                               size_t *aWhiteNodeSize,
>                               size_t *aPurpleBufferSize) const;
>  };
>  
> -class nsCycleCollectorRunner : public nsRunnable

Yay!

@@ +2738,5 @@
> +
> +    FixGrayBits(wantAllTraces);
> +
> +    nsAutoTArray<PtrInfo*, 4000> whiteNodes;
> +    if (!PrepareForCollection(aResults, &whiteNodes))

While you are touching this code anyways, please add braces around the bodies of all of the ifs in this function.

@@ +2743,5 @@
> +        return;
> +
> +    FreeSnowWhite(true);
> +
> +    MOZ_ASSERT(!mListener, "Should have cleared this already!");

Don't need this assert.

@@ +2746,5 @@
> +
> +    MOZ_ASSERT(!mListener, "Should have cleared this already!");
> +    if (aListener && NS_FAILED(aListener->Begin()))
> +        aListener = nullptr;
> +    mListener = aListener;

Don't need this assignment, just use aListener directly.

@@ +2748,5 @@
> +    if (aListener && NS_FAILED(aListener->Begin()))
> +        aListener = nullptr;
> +    mListener = aListener;
> +
> +    mCollected = BeginCollection(aCCType, mListener);

Rather than adding some kind of |collected| local variable, just early return if BeginCollection returns false.

@@ +2750,5 @@
> +    mListener = aListener;
> +
> +    mCollected = BeginCollection(aCCType, mListener);
> +
> +    mListener = nullptr;

Don't need this assignment.

Comment 6

[Andrew McCreight [:mccr8]]

(In reply to ben turner [:bent] from comment #4)
> (One quibble: the JS heap isn't nearly as big a space as all the objects
> reachable via the Traverse hooks, right? It bet it depends a lot on the way
> the browser is being used).
That's true, I guess the CC can trace the JS heap.  But in average use, according to telemetry, we spend something like two orders of magnitude less total time in the CC than in the GC, which may or may not mean anything.

> This code in particular is not something that I care a great deal about (and
> this will be the last time I speak in its defense) but in general I think we
> shouldn't be changing perf-sensitive code without even attempting to measure.
Yeah, it doesn't make me feel great, but in two years of cycle collector improvements I don't think we've ever noticed any effect on benchmarks, which suggests to me that any effects of the CC on performance are subtle at best.  But, as you say, we don't really know.

Comment 7

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

CC may trace the whole JS heap in theory, but black-bit-propagation during forgetSkippable tends
to protect us from that rather effectively.

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30ef08a0a1bc

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/30ef08a0a1bc