Split up the GC helper thread into separate and independent HelperThread tasks

RESOLVED FIXED in mozilla36

Status

()

Core
JavaScript: GC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

(Blocks: 1 bug)

Trunk
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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

4 years ago
Created attachment 8504311 [details] [diff] [review]
1_split_out_bg_alloc-v0.diff

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

4 years ago
Created attachment 8504321 [details] [diff] [review]
2_lockless_background_alloc-v0.diff

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

4 years ago
Created attachment 8504337 [details] [diff] [review]
2_lockless_background_alloc-v1.diff

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

4 years ago
I missed DEBUG guards around an isLocked usage:
https://tbpl.mozilla.org/?tree=Try&rev=4173bb6be587
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 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

4 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
https://hg.mozilla.org/mozilla-central/rev/682b11c70169
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Assignee)

Comment 10

4 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.