988486 - Split out GC state from JSRuntime

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

Right now the garbage collector has a lot of state that is all stored as public members on the JSRuntime.

This should be split out to a separate class, preferably with some kind of encapsulation so it's not all public.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: 0-split-out-page-allocator

For warmup, split out page allocator state.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: 1-split-out-gc-state

Split out GC state into new Collector class.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: 2-remove-gc-prefix

Remove 'gc' prefix from field names.

Comment 4

[Jon Coppeard (:jonco)]

Attachment: 3-use-methods-in-jsgc WIP

Work in progress, convert static collector functions taking JSRuntime* into methods of the new class.

Comment 5

[Jon Coppeard (:jonco)]

Attachment: 0-split-out-page-allocator

Rather than keep this as a stack of patches that keep getting bitrotted, I'm going to try and check this in as I go.

This patch just factors out the state for page allocation into its own class.

Comment 6

[Jon Coppeard (:jonco)]

Attachment: 1-split-out-gc-state

This is a big patch to factor out GC state into its own class.  I'm asking for feedback first to make sure people are happy with it, but feel free to r+ if you think it's ok.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8412702 [details] [diff] [review]
1-split-out-gc-state

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

This seems really nice! I kind of prefer the name "GC" to "Collector" though.

Perhaps in the future we could change all the functions in jsgc.cpp to take a GC* instead of a JSRuntime*. We'd still probably need a way to get from a GC to a JSRuntime, but at least then we'd be able to identify all the places where we use non-GC state from the GC. My hope was always to make the GC more generic so that it doesn't need to know the details of JSObject, Shape, etc. We're a long way off from there though.

Comment 8

[Jan de Mooij [:jandem]]

FWIW, the JITs have JitCompartment/JitZone/JitRuntime classes and TI has TypeZone. Maybe GCRuntime?

Comment 9

[Terrence Cole [:terrence]]

Comment on attachment 8412699 [details] [diff] [review]
0-split-out-page-allocator

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

r=me

Don't forget the markPagesUnused in Nursery.h: it's #ifndef XP_MACOSX.

::: js/src/gc/Memory.cpp
@@ +320,5 @@
>  #define NEED_PAGE_ALIGNED 0
>      size_t pa_start; // Page aligned starting
>      size_t pa_end; // Page aligned ending
>      size_t pa_size; // Total page aligned size
>      size_t page_size = sysconf(_SC_PAGESIZE); // Page size

Maybe file a followup to use pageSize here?

@@ +368,3 @@
>  {
>      void *pa_start; // Page aligned starting
>      size_t page_size = sysconf(_SC_PAGESIZE); // Page size

And here.

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 8412702 [details] [diff] [review]
1-split-out-gc-state

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

f+

::: js/src/gc/Collector.h
@@ +65,5 @@
> +        return !!nativeStackTop;
> +    }
> +};
> +
> +class Collector

I agree with Bill that this should be GC.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Actually, I like Jan's idea. GCRuntime sounds good.

Comment 12

[Jon Coppeard (:jonco)]

Attachment: 1-split-out-gc-state

Ok, GCRuntime it is.

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 8414533 [details] [diff] [review]
1-split-out-gc-state

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

\o/

::: js/src/gc/GCRuntime.h
@@ +2,5 @@
> + * vim: set ts=8 sts=4 et sw=4 tw=99:
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Extra space.

Comment 14

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/602ddd08eab9
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e6abdf3b4b4

Comment 15

[Carsten Book [:Tomcat]]

sorry jon, had to backout for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=38794444&tree=Mozilla-Inbound

Comment 16

[Jon Coppeard (:jonco)]

Fixed bustage and relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/39d2e19acdd5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b82db9273f3

Comment 17

[Carsten Book [:Tomcat]]

sorry had to backout again for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=38800628&tree=Mozilla-Inbound

Comment 18

[Jon Coppeard (:jonco)]

Ugh, I think pushed the wrong version of this to try when testing.

Really fixed the bustage and re-re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0beb424c86e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd9b3cd32b47

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b0beb424c86e
https://hg.mozilla.org/mozilla-central/rev/fd9b3cd32b47

Comment 20

[Jon Coppeard (:jonco)]

Attachment: 2-use-methods-in-jsgc

Here's a patch to convert static functions in GC implementation that take a JSRuntime* to methods on the GCRuntime class.

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 8418080 [details] [diff] [review]
2-use-methods-in-jsgc

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

Great work! Sorry about the delay, I could have sworn I hit submit.

::: js/src/gc/GCRuntime.h
@@ +93,5 @@
> +    void finish();
> +
> +    void setGCZeal(uint8_t zeal, uint32_t frequency);
> +    template <typename T>
> +    bool addRoot(T *rp, const char *name, JSGCRootType rootType);

I guess this should be on the same line as template <typename T> if we are packing these with no space.

@@ +120,5 @@
> +    Chunk *pickChunk(Zone *zone);
> +
> +    inline bool wantBackgroundAllocation() const;
> +
> +  private:

I guess the second private isn't required. Maybe use a comment if you want to demarcate a new section, or just leave a newline.

@@ +162,3 @@
>  
>    public:  // Internal state, public for now
> +    JSRuntime           *rt;

In general I'm against aligning members; however, if you're going to align this please make it the same as the other aligned things adjacent to it.

::: js/src/gc/Nursery.h
@@ +289,5 @@
>      void shrinkAllocableSpace();
>  
>      static void MinorGCCallback(JSTracer *trc, void **thingp, JSGCTraceKind kind);
>  
> +  public:

Instead of tagging them public at the end of the class, please lift the methods up to the end of the big public block at the top.

Comment 22

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5edc899d2b2

Comment 23

[Ed Morley [:emorley]]

Unfortunately the followup didn't fix the rooting hazzard analysis failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=39367930&tree=Mozilla-Inbound

So I've had to back this out:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8013e0d3cfd6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6a8fcbc59766

Comment 24

[Jon Coppeard (:jonco)]

Fixed static analysis and re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bad7731a849

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7bad7731a849

Comment 26

[Jon Coppeard (:jonco)]

Attachment: 3-make-state-private-1

Here's a patch to make some of the GCRuntime state private, adding the necessary accessors.

Comment 27

[Jon Coppeard (:jonco)]

Attachment: 4-move-verifier-into-runtime

Move verifier functions into GCRuntime as these access lots of state, and also functions to enable and disable generational GC.

Comment 28

[Terrence Cole [:terrence]]

Comment on attachment 8423795 [details] [diff] [review]
3-make-state-private-1

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

Nice! r=me

::: js/src/gc/GCRuntime.h
@@ +504,5 @@
>      js::GCHelperThread helperThread;
>  
>      ConservativeGCData conservativeGC;
>  
> +    //friend class js::gc::Chunk; // todo: remove

I guess you succeeded.

::: js/src/jsgcinlines.h
@@ +351,5 @@
>  
>  #ifdef DEBUG
>          /* Assert that no GCs can occur while a ZoneCellIter is live. */
> +        gc = &zone->runtimeFromAnyThread()->gc;
> +        gc->disallowAlloc();

\o/ This now makes it trivial to implement a |class AutoDisallowAlloc|. Please move these checks into such a class and kill the #ifdef blocks.

Comment 29

[Terrence Cole [:terrence]]

Comment on attachment 8423800 [details] [diff] [review]
4-move-verifier-into-runtime

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

That's full of win. r=me

Comment 30

[Terrence Cole [:terrence]]

Attachment: reorganize_zone-v0.diff

I noticed when reviewing these patches that JS::Zone has gotten quite hairy recently. This is a quick cleanup to organize JS::Zone into a facsimile of sanity. In particular it groups things by visibility and logical relatedness and splits the data members into their own section, sorting them by size. This saves us 8 bytes per zone on x64: not a huge win, but effectively free.

Comment 31

[Terrence Cole [:terrence]]

Attachment: reorganize_zone-v1.diff

Whoops! Forgot to pull yesterdays changes before doing this and had to rebase over the new intra-zone stuff.

Comment 32

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31984278765e
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad2c38c6649

Comment 33

[Jon Coppeard (:jonco)]

Comment on attachment 8424109 [details] [diff] [review]
reorganize_zone-v1.diff

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

Great. This was on my todo list for refactoring, but I'm happy for you to take it off me :)

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/31984278765e
https://hg.mozilla.org/mozilla-central/rev/0ad2c38c6649

Comment 35

[Jon Coppeard (:jonco)]

Attachment: fix-non-threadsafe-builds

The previous patches broke builds where JS_THREADSAFE was not defined.

Comment 36

[Jan de Mooij [:jandem]]

Comment on attachment 8426135 [details] [diff] [review]
fix-non-threadsafe-builds

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

Looks good. (decoder asked to land this directly on m-c to unbreak the fuzzers...)

Comment 37

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/mozilla-central/rev/a6f36c31aa8f

Comment 38

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eca41f62ac61

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/eca41f62ac61

Comment 40

[Luke Wagner [:luke]]

Wow, nice!

Comment 41

[Paul Wright]

It looks like all 7 patches have landed.  Is the "leave open" still needed?

Comment 42

[Jon Coppeard (:jonco)]

(In reply to Paul Wright from comment #41)
There are more patches to come.

Comment 43

[Jon Coppeard (:jonco)]

Attachment: 5 - Move script counts out of GC

I'm not sure why this were part of the GC, apart from the fact that it needs marking.  I think the implementation makes more sense in jsopcode.cpp.

Comment 44

[Jon Coppeard (:jonco)]

Attachment: 6 - Make more state private

Make more state private and add the appropriate accessors, with some minor tidyup along the way.

Comment 45

[Terrence Cole [:terrence]]

Comment on attachment 8435788 [details] [diff] [review]
5 - Move script counts out of GC

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

Yeah, it's totally absurd that that was in jsgc.cpp.

Comment 46

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03eccac81e15
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a4a9062253

Comment 47

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-other crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6932ea846a28

https://tbpl.mozilla.org/php/getParsedLog.php?id=41274495&tree=Mozilla-Inbound

Comment 48

[Jon Coppeard (:jonco)]

Fixed and re-landed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4f7f54f2ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/18eea9cb1c46

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fc4f7f54f2ac
https://hg.mozilla.org/mozilla-central/rev/18eea9cb1c46

Comment 50

[Jon Coppeard (:jonco)]

Attachment: 7 - Make more state private

Comment 51

[Terrence Cole [:terrence]]

Comment on attachment 8439941 [details] [diff] [review]
7 - Make more state private

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

::: js/src/gc/GCRuntime.h
@@ +115,5 @@
>  
> +    // Performance note: if isFJMinorCollecting turns out to be slow
> +    // because reading the counter is slow then we may be able to
> +    // augment the counter with a volatile flag that is set iff the
> +    // counter is greater than zero.  (It will require some care to

One space.

@@ +520,4 @@
>  #ifdef JSGC_GENERATIONAL
>      js::Nursery           nursery;
>      js::gc::StoreBuffer   storeBuffer;
>  #endif

Please move these two decls up above the private section if they need to remain public. Given their size, they should probably be up at the top anyway.

Comment 52

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39acaa2e399b

Comment 53

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: Fix a build warning

Fixes a build warning due to storeBuffer and nursery in the class code.

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/39acaa2e399b

Comment 55

[Jon Coppeard (:jonco)]

Comment on attachment 8440708 [details] [diff] [review]
Fix a build warning

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

Great, thanks for fixing this.

Comment 56

[Benjamin Bouvier [:bbouvier] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d803f558e358

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d803f558e358

Comment 58

[Jon Coppeard (:jonco)]

Attachment: 8 - Make more state private

Latest refactoring installment - nearly there.

Comment 59

[Terrence Cole [:terrence]]

Comment on attachment 8445739 [details] [diff] [review]
8 - Make more state private

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

Nice!

::: js/src/gc/GCRuntime.h
@@ +467,5 @@
>       * creation.
>       */
>      uintptr_t             disableStrictProxyCheckingCount;
>  #else
>      uintptr_t             unused1;

O_o  I'm pretty sure that unused1 is totally unnecessary.

::: js/src/gc/Marking.cpp
@@ +224,2 @@
>  
> +        JS_ASSERT_IF(AsGCMarker(trc)->getMarkColor() == GRAY,

s/AsGCMarker(trc)/gcMarker/

Comment 60

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e599c809f50d

Comment 61

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e599c809f50d

Comment 62

[Jon Coppeard (:jonco)]

Attachment: 9 - Make more state private

Final patch for this to make GCRuntime state private.

Comment 63

[Terrence Cole [:terrence]]

Comment on attachment 8451044 [details] [diff] [review]
9 - Make more state private

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

\o/

::: js/src/gc/GCRuntime.h
@@ +33,5 @@
>  class MarkingValidator;
>  struct AutoPrepareForTracing;
>  class AutoTraceSession;
>  
> +class ChunkPool {

{ on newline.

@@ +40,5 @@
> +
> +  public:
> +    ChunkPool()
> +      : emptyChunkListHead(nullptr),
> +        emptyCount(0) { }

Brace pair on newline.

Comment 64

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/baec82f2baeb

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/baec82f2baeb