Closed Bug 1074942 Opened 8 years ago Closed 8 years ago

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


(Core :: JavaScript: GC, defect)

Not set





(Reporter: terrence, Assigned: terrence)


(Blocks 1 open bug)



(1 file, 2 obsolete files)

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.
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.
Attachment #8504311 - Flags: review?(bhackett1024)
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)
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)
I missed DEBUG guards around an isLocked usage:
Comment on attachment 8504311 [details] [diff] [review]

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())
> +    {


::: 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]

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+

Note that the failures in the try run appear to all be from part2, which is not landing here (or probably ever).
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8504337 [details] [diff] [review]

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.