Closed
Bug 988486
Opened 10 years ago
Closed 10 years ago
Split out GC state from JSRuntime
Categories
(Core :: JavaScript: GC, enhancement)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(13 files, 6 obsolete files)
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 |
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•10 years ago
|
||
For warmup, split out page allocator state.
Assignee | ||
Comment 2•10 years ago
|
||
Split out GC state into new Collector class.
Assignee | ||
Comment 3•10 years ago
|
||
Remove 'gc' prefix from field names.
Assignee | ||
Comment 4•10 years ago
|
||
Work in progress, convert static collector functions taking JSRuntime* into methods of the new class.
Assignee | ||
Comment 5•10 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•10 years ago
|
||
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•10 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+
Comment 8•10 years ago
|
||
FWIW, the JITs have JitCompartment/JitZone/JitRuntime classes and TI has TypeZone. Maybe GCRuntime?
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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•10 years ago
|
||
Ok, GCRuntime it is.
Attachment #8412702 -
Attachment is obsolete: true
Attachment #8414533 -
Flags: review?(terrence)
Comment 13•10 years ago
|
||
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 | ||
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/602ddd08eab9 https://hg.mozilla.org/integration/mozilla-inbound/rev/3e6abdf3b4b4
Assignee | ||
Updated•10 years ago
|
Whiteboard: [leave open]
Comment 15•10 years ago
|
||
sorry jon, had to backout for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=38794444&tree=Mozilla-Inbound
Assignee | ||
Comment 16•10 years ago
|
||
Fixed bustage and relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/39d2e19acdd5 https://hg.mozilla.org/integration/mozilla-inbound/rev/8b82db9273f3
Comment 17•10 years ago
|
||
sorry had to backout again for bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=38800628&tree=Mozilla-Inbound
Assignee | ||
Comment 18•10 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
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b0beb424c86e https://hg.mozilla.org/mozilla-central/rev/fd9b3cd32b47
Assignee | ||
Comment 20•10 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 21•10 years ago
|
||
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+
Assignee | ||
Comment 22•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5edc899d2b2
Comment 23•10 years ago
|
||
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
Assignee | ||
Comment 24•10 years ago
|
||
Fixed static analysis and re-landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/7bad7731a849
Assignee | ||
Comment 26•10 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•10 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 28•10 years ago
|
||
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 29•10 years ago
|
||
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+
Comment 30•10 years ago
|
||
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)
Comment 31•10 years ago
|
||
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 32•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/31984278765e https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad2c38c6649
Assignee | ||
Comment 33•10 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+
https://hg.mozilla.org/mozilla-central/rev/31984278765e https://hg.mozilla.org/mozilla-central/rev/0ad2c38c6649
Assignee | ||
Comment 35•10 years ago
|
||
The previous patches broke builds where JS_THREADSAFE was not defined.
Attachment #8426135 -
Flags: review?(jdemooij)
Comment 36•10 years ago
|
||
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+
Assignee | ||
Comment 37•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a6f36c31aa8f
Updated•10 years ago
|
Attachment #8424109 -
Flags: checkin+
Comment 40•10 years ago
|
||
Wow, nice!
Comment 41•10 years ago
|
||
It looks like all 7 patches have landed. Is the "leave open" still needed?
Assignee | ||
Comment 42•10 years ago
|
||
(In reply to Paul Wright from comment #41) There are more patches to come.
Assignee | ||
Comment 43•10 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•10 years ago
|
||
Make more state private and add the appropriate accessors, with some minor tidyup along the way.
Attachment #8435789 -
Flags: review?(terrence)
Comment 45•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8435789 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 46•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/03eccac81e15 https://hg.mozilla.org/integration/mozilla-inbound/rev/14a4a9062253
Comment 47•10 years ago
|
||
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
Assignee | ||
Comment 48•10 years ago
|
||
Fixed and re-landed. https://hg.mozilla.org/integration/mozilla-inbound/rev/fc4f7f54f2ac https://hg.mozilla.org/integration/mozilla-inbound/rev/18eea9cb1c46
Comment 49•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fc4f7f54f2ac https://hg.mozilla.org/mozilla-central/rev/18eea9cb1c46
Assignee | ||
Comment 50•10 years ago
|
||
Attachment #8439941 -
Flags: review?(terrence)
Comment 51•10 years ago
|
||
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+
Assignee | ||
Comment 52•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/39acaa2e399b
Comment 53•10 years ago
|
||
Fixes a build warning due to storeBuffer and nursery in the class code.
Attachment #8440708 -
Flags: review?(jcoppeard)
Comment 54•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/39acaa2e399b
Flags: in-testsuite+
Assignee | ||
Comment 55•10 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•10 years ago
|
||
Latest refactoring installment - nearly there.
Attachment #8445739 -
Flags: review?(terrence)
Assignee | ||
Updated•10 years ago
|
Attachment #8445739 -
Attachment description: more-refactoring-2 → 8 - Make more state private
Comment 59•10 years ago
|
||
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 60•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e599c809f50d
Assignee | ||
Comment 62•10 years ago
|
||
Final patch for this to make GCRuntime state private.
Attachment #8451044 -
Flags: review?(terrence)
Comment 63•10 years ago
|
||
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+
Assignee | ||
Comment 64•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/baec82f2baeb
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/baec82f2baeb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•