Closed
Bug 1074942
Opened 8 years ago
Closed 8 years ago
Split up the GC helper thread into separate and independent HelperThread tasks
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: terrence, Assigned: terrence)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
17.03 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
The state management going on in GCHelperThread is gnarly because it used to be our only thread and it needed to handle 3 different tasks. Now that we have a good work-stealing threading implementation, we can easily split it up into 3 separate tasks and manage them independently, totally eliminating the gnarly state machine.
Assignee | ||
Comment 1•8 years ago
|
||
As it says on the tin: makes bg alloc a GCParallelTask. The only new mechanism here is cancellation -- I've done this in a generic way because I think it will be handy in other cases later and doesn't add any overhead when not used. https://tbpl.mozilla.org/?tree=Try&rev=7d9544b939ca
Attachment #8504311 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 2•8 years ago
|
||
It turns out we don't actually need to take the GC lock in the loop. It makes wantBackgroundAlloc racy, but we don't really care about that: worst case we get a few too few or too many chunks. More relevantly, with Bug 1074961 done, we can pass in direct const-refs to the values we're reading off-thread and unfriend it from the GCRuntime entirely -- the amount of damage we can do with this setup is now strictly bounded.
Attachment #8504321 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 3•8 years ago
|
||
Fix some bustage in steal and builds on non-linux platforms.
Attachment #8504321 -
Attachment is obsolete: true
Attachment #8504321 -
Flags: review?(bhackett1024)
Attachment #8504337 -
Flags: review?(bhackett1024)
Assignee | ||
Comment 4•8 years ago
|
||
I missed DEBUG guards around an isLocked usage: https://tbpl.mozilla.org/?tree=Try&rev=4173bb6be587
Assignee | ||
Comment 5•8 years ago
|
||
Oh, ffs: https://tbpl.mozilla.org/?tree=Try&rev=5359d4cf18da
Comment 6•8 years ago
|
||
Comment on attachment 8504311 [details] [diff] [review] 1_split_out_bg_alloc-v0.diff Review of attachment 8504311 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsgc.cpp @@ +3057,1 @@ > } you can remove the {} now @@ +3160,5 @@ > + AutoTraceLog logAllocation(logger, TraceLogger::GCAllocation); > + > + AutoLockGC lock(runtime); > + while (!cancel_ && runtime->gc.wantBackgroundAllocation()) > + { formatting ::: js/src/jsgc.h @@ +1103,5 @@ > public: > void runFromHelperThread(); > + > + private: > + static bool outOfLineHelperThreadIsLocked(); I think it would be better to put the isRunning() implementation in HelperThreads.cpp so you can avoid this method.
Attachment #8504311 -
Flags: review?(bhackett1024) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8504337 [details] [diff] [review] 2_lockless_background_alloc-v1.diff Review of attachment 8504337 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/GCRuntime.h @@ +47,3 @@ > > const bool enabled_; > + const size_t &emptyCount_; Comment that these sizes are read from without a lock held during wantBackgroundAllocation(), and can race. I think it would be better too if these were const size_t*, to make it more obvious where the racy reads are happening. @@ +53,5 @@ > + > + // If the heap has fewer chunks than this, we'll skip background > + // allocation to save memory when heap utilization is low. > + const static size_t MinChunksForBackgroundAlloc = 4; > + bool canBackgroundAllocate() const { This method is only called by wantBackgroundAllocation(), so this logic should be folded in there so that only one method accesses the racy fields.
Attachment #8504337 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Comment 8•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/682b11c70169 Note that the failures in the try run appear to all be from part2, which is not landing here (or probably ever). https://tbpl.mozilla.org/?tree=Try&rev=5359d4cf18da
Comment 9•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/682b11c70169
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8504337 [details] [diff] [review] 2_lockless_background_alloc-v1.diff It's a neat idea, but I don't think it's worth the complexity, given that it's not really causing any problems right now.
Attachment #8504337 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•