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

Status: RESOLVED FIXED

Description

[Terrence Cole [:terrence]]

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.

Comment 1

[Terrence Cole [:terrence]]

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

Comment 2

[Terrence Cole [:terrence]]

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.

Comment 3

[Terrence Cole [:terrence]]

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

Fix some bustage in steal and builds on non-linux platforms.

Comment 4

[Terrence Cole [:terrence]]

I missed DEBUG guards around an isLocked usage:
https://tbpl.mozilla.org/?tree=Try&rev=4173bb6be587

Comment 5

[Terrence Cole [:terrence]]

Oh, ffs:
https://tbpl.mozilla.org/?tree=Try&rev=5359d4cf18da

Comment 6

[Brian Hackett (:bhackett)]

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.

Comment 7

[Brian Hackett (:bhackett)]

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.

Comment 8

[Terrence Cole [:terrence]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/682b11c70169

Comment 10

[Terrence Cole [:terrence]]

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.