Split out GC state from JSRuntime

RESOLVED FIXED in mozilla33

Status

()

enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(13 attachments, 6 obsolete attachments)

25.07 KB, patch
terrence
: review+
Details | Diff | Splinter Review
276.32 KB, patch
terrence
: review+
Details | Diff | Splinter Review
108.31 KB, patch
terrence
: review+
Details | Diff | Splinter Review
31.14 KB, patch
terrence
: review+
Details | Diff | Splinter Review
20.05 KB, patch
terrence
: review+
Details | Diff | Splinter Review
23.39 KB, patch
jonco
: review+
terrence
: checkin+
Details | Diff | Splinter Review
1.67 KB, patch
jandem
: review+
Details | Diff | Splinter Review
14.86 KB, patch
terrence
: review+
Details | Diff | Splinter Review
47.45 KB, patch
terrence
: review+
Details | Diff | Splinter Review
59.13 KB, patch
terrence
: review+
Details | Diff | Splinter Review
1.42 KB, patch
jonco
: review+
Details | Diff | Splinter Review
54.32 KB, patch
terrence
: review+
Details | Diff | Splinter Review
24.87 KB, patch
terrence
: review+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
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.
Assignee

Comment 1

5 years ago
Posted patch 0-split-out-page-allocator (obsolete) — Splinter Review
For warmup, split out page allocator state.
Assignee

Comment 2

5 years ago
Posted patch 1-split-out-gc-state (obsolete) — Splinter Review
Split out GC state into new Collector class.
Assignee

Comment 3

5 years ago
Posted patch 2-remove-gc-prefix (obsolete) — Splinter Review
Remove 'gc' prefix from field names.
Assignee

Comment 4

5 years ago
Posted patch 3-use-methods-in-jsgc WIP (obsolete) — Splinter Review
Work in progress, convert static collector functions taking JSRuntime* into methods of the new class.
Assignee

Comment 5

5 years ago
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.
Attachment #8408380 - Attachment is obsolete: true
Attachment #8412699 - Flags: review?(terrence)
Assignee

Comment 6

5 years ago
Posted patch 1-split-out-gc-state (obsolete) — Splinter Review
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.
Attachment #8408381 - Attachment is obsolete: true
Attachment #8408382 - Attachment is obsolete: true
Attachment #8412702 - Flags: feedback?(terrence)
Assignee

Updated

5 years ago
Attachment #8412702 - Flags: feedback?(wmccloskey)
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.
Attachment #8412702 - Flags: feedback?(wmccloskey) → feedback+
FWIW, the JITs have JitCompartment/JitZone/JitRuntime classes and TI has TypeZone. Maybe GCRuntime?
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.
Attachment #8412699 - Flags: review?(terrence) → review+
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.
Attachment #8412702 - Flags: feedback?(terrence) → feedback+
Actually, I like Jan's idea. GCRuntime sounds good.
Assignee

Comment 12

5 years ago
Ok, GCRuntime it is.
Attachment #8412702 - Attachment is obsolete: true
Attachment #8414533 - Flags: review?(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.
Attachment #8414533 - Flags: review?(terrence) → review+
Assignee

Updated

5 years ago
Whiteboard: [leave open]
Assignee

Comment 18

5 years ago
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
Assignee

Comment 20

5 years ago
Here's a patch to convert static functions in GC implementation that take a JSRuntime* to methods on the GCRuntime class.
Attachment #8408384 - Attachment is obsolete: true
Attachment #8418080 - Flags: review?(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.
Attachment #8418080 - Flags: review?(terrence) → review+
Depends on: 1009788
Assignee

Comment 26

5 years ago
Here's a patch to make some of the GCRuntime state private, adding the necessary accessors.
Attachment #8423795 - Flags: review?(terrence)
Assignee

Comment 27

5 years ago
Move verifier functions into GCRuntime as these access lots of state, and also functions to enable and disable generational GC.
Attachment #8423800 - Flags: review?(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.
Attachment #8423795 - Flags: review?(terrence) → review+
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
Attachment #8423800 - Flags: review?(terrence) → review+
Posted patch reorganize_zone-v0.diff (obsolete) — Splinter Review
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.
Attachment #8424053 - Flags: review?(jcoppeard)
Whoops! Forgot to pull yesterdays changes before doing this and had to rebase over the new intra-zone stuff.
Attachment #8424053 - Attachment is obsolete: true
Attachment #8424053 - Flags: review?(jcoppeard)
Attachment #8424109 - Flags: review?(jcoppeard)
Assignee

Comment 33

5 years ago
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 :)
Attachment #8424109 - Flags: review?(jcoppeard) → review+
Assignee

Comment 35

5 years ago
The previous patches broke builds where JS_THREADSAFE was not defined.
Attachment #8426135 - Flags: review?(jdemooij)
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...)
Attachment #8426135 - Flags: review?(jdemooij) → review+
Attachment #8424109 - Flags: checkin+
Wow, nice!

Comment 41

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

Comment 42

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

Comment 43

5 years ago
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.
Attachment #8435788 - Flags: review?(terrence)
Assignee

Comment 44

5 years ago
Make more state private and add the appropriate accessors, with some minor tidyup along the way.
Attachment #8435789 - Flags: review?(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.
Attachment #8435788 - Flags: review?(terrence) → review+
Attachment #8435789 - Flags: review?(terrence) → review+
Assignee

Comment 50

5 years ago
Attachment #8439941 - Flags: review?(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.
Attachment #8439941 - Flags: review?(terrence) → review+
Fixes a build warning due to storeBuffer and nursery in the class code.
Attachment #8440708 - Flags: review?(jcoppeard)
Assignee

Comment 55

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

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

Great, thanks for fixing this.
Attachment #8440708 - Flags: review?(jcoppeard) → review+
Assignee

Comment 58

5 years ago
Latest refactoring installment - nearly there.
Attachment #8445739 - Flags: review?(terrence)
Assignee

Updated

5 years ago
Attachment #8445739 - Attachment description: more-refactoring-2 → 8 - Make more state private
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/
Attachment #8445739 - Flags: review?(terrence) → review+
Assignee

Comment 62

5 years ago
Final patch for this to make GCRuntime state private.
Attachment #8451044 - Flags: review?(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.
Attachment #8451044 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/baec82f2baeb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1037516
Depends on: 1023035
You need to log in before you can comment on or make changes to this bug.