Remove support for the CC thread

RESOLVED FIXED in mozilla26

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

(Blocks: 1 bug)

unspecified
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 785891 [details] [diff] [review]
Patch

We're not using it, let's get rid of it.
Attachment #785891 - Flags: review?(continuation)

Comment 1

5 years ago
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 :)
Created attachment 785942 [details] [diff] [review]
Patch

This removes the consumers of JS_SetRuntimeThread.  I don't want to remove the API in this bug.
Attachment #785891 - Attachment is obsolete: true
Attachment #785891 - Flags: review?(continuation)
Attachment #785942 - Flags: review?(continuation)
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.
(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 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.
Attachment #785942 - Flags: review?(continuation) → review+
(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

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

Updated

5 years ago
Blocks: 901934
Blocks: 749370
https://hg.mozilla.org/mozilla-central/rev/30ef08a0a1bc
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

Updated

5 years ago
Depends on: 905920

Updated

5 years ago
No longer depends on: 905920
Blocks: 711235
Blocks: 908462
Blocks: 902775
You need to log in before you can comment on or make changes to this bug.