Closed
Bug 1452982
Opened 6 years ago
Closed 6 years ago
Some JSContext/JSRuntime/ZoneGroup cleanup
Categories
(Core :: JavaScript Engine, enhancement, P3)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(15 files)
31.64 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
12.03 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
49.86 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
22.73 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
2.56 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
60.95 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
15.23 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
2.56 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
22.86 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
73.59 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
43.99 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
6.10 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
8.82 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
51.09 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
9.37 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•6 years ago
|
||
We can do this since bug 1449135. Benefits are: (1) a bit faster (2) in debug builds we assert we're on the runtime's main thread. This also removes some unused code: JS::isGCEnabled and GCRuntime::canChangeActiveContext.
Attachment #8966601 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 2•6 years ago
|
||
The plan is to remove ZoneGroup now that cooperative scheduling is gone. The debuggerList can just move back to JSRuntime (all places where we iterated over this already looked at all zone groups so this should be equivalent).
Attachment #8966619 -
Flags: review?(jcoppeard)
Comment 3•6 years ago
|
||
Comment on attachment 8966601 [details] [diff] [review] Part 1 - Use rt->mainContextFromOwnThread() instead of TlsContext.get() Review of attachment 8966601 [details] [diff] [review]: ----------------------------------------------------------------- Great, thanks for doing this.
Attachment #8966601 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Attachment #8966619 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•6 years ago
|
||
This removes ZoneGroup::nursery() and ZoneGroup::storeBuffer(). These just forwarded to JSRuntime*, but it's an extra indirection and note that we even had some code that still assumed nursery per ZoneGroup by looping over all groups!
Attachment #8966706 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 5•6 years ago
|
||
This is mostly just moving code, except: * I made ionLazyLinkList_ and ionLazyLinkListSize_ use ActiveThreadData instead of HelperThreadLockData. Asserting they're only accessed on the main thread is a stronger invariant. (There are some now-redundant asserts in some methods that I kept.) * numFinishedBuilders is no longer a public member - I added numFinishedBuildersRef that takes the helper thread lock and numFinishedBuilders to return the value unlocked to document the invariants better.
Attachment #8966872 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 6•6 years ago
|
||
Attachment #8966877 -
Flags: review?(jcoppeard)
Comment 7•6 years ago
|
||
Comment on attachment 8966706 [details] [diff] [review] Part 3 - Remove ZoneGroup GC methods Review of attachment 8966706 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/TypedObject.cpp @@ +1433,5 @@ > > // Trigger a post barrier when attaching an object outside the nursery to > // one that is inside it. > if (owner && !IsInsideNursery(this) && IsInsideNursery(owner)) > + runtimeFromActiveCooperatingThread()->gc.storeBuffer().putWholeCell(this); There's also a storeBuffer() method on Cell that returns the store buffer for cells that are in the nursery, so in cases like this you could write: owner->storeBuffer()->putWholeCell(...) ::: js/src/gc/GC.cpp @@ -1066,5 @@ > > ZealMode zealMode = ZealMode(zeal); > - if (zealMode == ZealMode::GenerationalGC) { > - for (ZoneGroupsIter group(rt); !group.done(); group.next()) > - group->nursery().enterZealMode(); Thanks for fixing that :)
Attachment #8966706 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Attachment #8966877 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Attachment #8966872 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 8•6 years ago
|
||
This removes ZoneGroup. The remaining fields were moved into GCRuntime or Zone. I'll remove the almost-empty ZoneGroup.{h,cpp} files separately to avoid weird unified build issues. Also, ZoneGroupData should be renamed to ZoneData.
Attachment #8966940 -
Flags: review?(jcoppeard)
Updated•6 years ago
|
Attachment #8966940 -
Flags: review?(jcoppeard) → review+
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/473b7d381544 part 1 - Use rt->mainContextFromOwnThread() instead of TlsContext.get() in some places. r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/ae45e56c3c71 part 2 - Move debuggerList from ZoneGroup to JSRuntime. r=jonco
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Assignee | ||
Comment 10•6 years ago
|
||
Attachment #8967313 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 11•6 years ago
|
||
Oh that latest patch does not rename some related things like ZoneGroupOrGCTaskData, I'll rename these separately.
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #8967316 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 13•6 years ago
|
||
I searched for ZoneGroup and "zone group" in js/src and fixed them.
Attachment #8967320 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 14•6 years ago
|
||
Attachment #8967322 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 15•6 years ago
|
||
I'll stop here for now to avoid bitrot, but after this lands there will be a few more renamings (ActiveThread -> MainThread etc) and some comment fixes.
Updated•6 years ago
|
Attachment #8967313 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Attachment #8967316 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Attachment #8967320 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Attachment #8967322 -
Flags: review?(jcoppeard) → review+
Comment 16•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f8104a9c57c8 part 3 - Remove ZoneGroup nursery/storeBuffer methods. r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/17debfcdc775 part 4 - Move jit-related fields from ZoneGroup to JitRuntime. r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/a56736bcd64a part 5 - Refactor JitRuntime::isOptimizationTrackingEnabled to take a JSRuntime* instead of ZoneGroup*. r=jonco
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #7) > There's also a storeBuffer() method on Cell that returns the store buffer > for cells that are in the nursery, so in cases like this you could write: > > owner->storeBuffer()->putWholeCell(...) Good idea, I did that in a few places.
Comment 18•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/473b7d381544 https://hg.mozilla.org/mozilla-central/rev/ae45e56c3c71
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f8104a9c57c8 https://hg.mozilla.org/mozilla-central/rev/17debfcdc775 https://hg.mozilla.org/mozilla-central/rev/a56736bcd64a
Comment 20•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/73fa6225a03f part 6 - Remove ZoneGroup. r=jonco
Comment 21•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/73fa6225a03f
Comment 22•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d38e3ca7457a part 7 - Rename ZoneGroupData to ZoneData. r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/35d142c5c599 part 8 - Remove ZoneGroup.h/cpp files. r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/a434fac58370 part 9 - Remove/rename remaining ZoneGroup references. r=jonco
Comment 23•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d38e3ca7457a https://hg.mozilla.org/mozilla-central/rev/35d142c5c599 https://hg.mozilla.org/mozilla-central/rev/a434fac58370
Comment 24•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/16fcaa62dee9 part 10 - Rename *ActiveCooperatingThread to *MainThread. r=jonco
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/16fcaa62dee9
Assignee | ||
Comment 26•6 years ago
|
||
Attachment #8968149 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 27•6 years ago
|
||
Attachment #8968153 -
Flags: review?(jcoppeard)
Updated•6 years ago
|
Attachment #8968149 -
Flags: review?(jcoppeard) → review+
Updated•6 years ago
|
Attachment #8968153 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 28•6 years ago
|
||
Attachment #8968166 -
Flags: review?(jcoppeard)
Comment 29•6 years ago
|
||
Comment on attachment 8968166 [details] [diff] [review] Part 13 - Misc cleanup Review of attachment 8968166 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/TraceLogging.cpp @@ -879,5 @@ > /*NOTREACHED*/ > } > > - if (strstr(options, "EnableActiveThread")) > - cooperatingThreadEnabled = true; Is this going to break any tools?
Attachment #8968166 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 30•6 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #29) > Is this going to break any tools? Good question. I don't expect any breakage (TraceLogger isn't used much) but if it does happen we can either fix the tools or support both values.
Comment 31•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb5e6101f4a part 11 - Rename ActiveThread to MainThread. r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/d46b75deaae5 part 12 - Clean up ContextKind and CheckThreadLocal. r=jonco
Comment 32•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb369e8ad78 part 13 - Misc cleanup related to cooperative scheduling. r=jonco
Assignee | ||
Comment 33•6 years ago
|
||
Attachment #8968481 -
Flags: review?(jcoppeard)
Assignee | ||
Comment 34•6 years ago
|
||
THREAD_TYPE_COOPERATING => THREAD_TYPE_MAIN MAX_ACTIVE_THREAD_SCRIPT_SIZE => MAX_MAIN_THREAD_SCRIPT_SIZE MAX_ACTIVE_THREAD_LOCALS_AND_ARGS => MAX_MAIN_THREAD_LOCALS_AND_ARGS
Attachment #8968483 -
Flags: review?(jcoppeard)
Comment 35•6 years ago
|
||
Comment on attachment 8968481 [details] [diff] [review] Part 14 - Rename "active thread" to "main thread" Review of attachment 8968481 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/GCRuntime.h @@ +935,5 @@ > #ifdef DEBUG > MainThreadData<bool> arenasEmptyAtShutdown; > #endif > > + /* Synchronize GC heap access among GC helper threads and main threads. */ Should be "and the main thread." ::: js/src/jit/JitOptions.h @@ +20,2 @@ > static const uint32_t MAX_ACTIVE_THREAD_SCRIPT_SIZE = 2 * 1000; > static const uint32_t MAX_ACTIVE_THREAD_LOCALS_AND_ARGS = 256; I guess these MAX_ACTIVE_THREAD... constants should be renamed too. ::: js/src/vm/HelperThreads.cpp @@ +1759,5 @@ > AutoUnlockHelperThreadState unlock(locked); > wasm::ExecuteCompileTaskFromHelperThread(task); > } > > + // No main thread should be waiting on the CONSUMER mutex. I originally commented that this should be "The main thread should not", but helper threads can be used by multiple runtimes so maybe this does make sense. @@ +1802,5 @@ > task->execute(); > task->dispatchResolveAndDestroy(); > } > > + // No main thread should be waiting on the CONSUMER mutex. Ditto above.
Attachment #8968481 -
Flags: review?(jcoppeard) → review+
Comment 36•6 years ago
|
||
Comment on attachment 8968483 [details] [diff] [review] Part 15 - Rename some constants Review of attachment 8968483 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/JitOptions.h @@ +17,5 @@ > > // Longer scripts can only be compiled off thread, as these compilations > // can be expensive and stall the main thread for too long. > +static const uint32_t MAX_MAIN_THREAD_SCRIPT_SIZE = 2 * 1000; > +static const uint32_t MAX_MAIN_THREAD_LOCALS_AND_ARGS = 256; Oh, you fixed those here, please ignore my previous comment.
Attachment #8968483 -
Flags: review?(jcoppeard) → review+
Comment 37•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7cb5e6101f4a https://hg.mozilla.org/mozilla-central/rev/d46b75deaae5 https://hg.mozilla.org/mozilla-central/rev/4eb369e8ad78
Comment 38•6 years ago
|
||
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2f7d0134b221 part 14 - Rename 'active thread' to 'main thread'. r=jonco https://hg.mozilla.org/integration/mozilla-inbound/rev/292f8e5c6336 part 15 - Rename some constants. r=jonco
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 39•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2f7d0134b221 https://hg.mozilla.org/mozilla-central/rev/292f8e5c6336
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•