Bug 164441 – TypeProfiler and running GC collection on another thread don't play nicely with each other

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Saam Barati]

the type profiler wants to do some JS like work, and call into getOwnPropertySlot, which may allocate stuff, etc.
The GC should communicate to the mutator thread that it should do this stuff before it stops.

[Saam Barati]

<rdar://problem/29132174>

[Filip Pizlo]

I think that we could make this very clean.  Basically we want to replace all these darn bits with something that allows the collector to supply a lambda of execution to the mutator and then wait for it to do that.

We could do it with these additional fields in Heap:

    Lock m_requestedActivityLock;
    RefPtr<SharedTask<void()>> m_requestedActivity;

We would replace the needFinalizeBit with a requestedActivityBit, and rename finalize() to performRequestedActivity(), which would do:

    clearRequestedActivity(); // CAS loop to clear activity bit and unparkAll().
    {
        LockHolder locker(m_requestedActivityLock);
        if (m_requestedActivity) {
            m_requestedActivity->run();
            m_requestedActivity = nullptr;
        }
    }

In order to request an activity, the collector would do:

    template<typename Func>
    void requestActivity(const Func& func)
    {
        {
            LockHolder locker(m_requestedActivityLock);
            RefPtr<SharedTask<void()>> previousActivity = m_requestedActivity;
            m_requestedActivity = createSharedTask<void()>(
                [=] () {
                    if (previousActivity)
                        previousActivity->run();
                    func();
                });
        }
        setRequestedActivity(); // CAS loop to set activity bit and unparkAll().
    }

I think that this is safe.  There are two atomic steps here on both sides.  In parentheses, I put short names for these atomic steps.

mutator: #1 clear activity bit (M:clear)
mutator: #2 run and clear the activity (M:run)
collector: #1 set the activity (C:setActivity)
collector: #2 set the activity bit (c:setBit)

The two obviously safe executions are:

mutator first: M:clear, M:run, C:setActivity, C:setBit

and

collector first: C:setActivity, C:setBit, M:clear, M:run

We know that each of these steps is atomic and has all of the fences necessary to not be magically reordered.  So, we just need to consider if the following interleavings are valid:

M:clear, C:setActivity, C:setBit, M:run
M:clear, C:setActivity, M:run, C:setBit
C:setActivity, M:clear, M:run, C:setBit
C:setActivity, M:clear, C:setBit, M:run

I don't think there are other interleavings. So let's consider these weird ones! They will be obviously correct if they end up behaving like mutator first or collector first.

M:clear, C:setActivity, C:setBit, M:run: The mutator will run the activities that we requested, but will not clear the bit. That means that the mutator will take a slow path next time it looks at the worldState, and and then it will try to run the bogus activity, but thanks to the null check it will just clear the bit.  So, this behaves just like collector first.

M:clear, C:setActivity, M:run, C:setBit: The mutator will run the activities that we requested, but will not clear the bit. So, this behaves just like collector first.

C:setActivity, M:clear, M:run, C:setBit: The mutator will run the activities that we requested, but will not clear the bit. So, this behaves just like collector first.

C:setActivity, M:clear, C:setBit, M:run: The mutator will run the activities that we requested, but will not clear the bit. So, this behaves just like collector first.

As far as I can tell, there is no way that the collector would request an activity and the mutator would fail to run it prior to regaining heap access. This is true because the only way for the mutator to clear the bit is if the collector runs first without any interleavings. In all other modes, the mutator will leave the bit set. So, the existing logic in stopTheWorldSlow() and friends for dealing with the needFinalizeBit can be reused for this more general requestedActivityBit. We can be sure that so long as the bit is set the mutator will not sleep but will instead try to run the activity. When it does so, it is guaranteed to only clear the bit if it ran the activity without any collector interleaving. If the collector sets an activity before the mutator runs it, the mutator will run it. If the collector sets an activity after the mutator runs the last one, the mutator won't sleep, release heap access, or acquire heap access until it runs that activity.

This would let us combine needFinalizeBit, gcDidJITBit, and whatever new bit you'll need for this.

Then we can convert waitWhileNeedFinalize() into waitForRequestedActivityCompletion(), so that the GC doesn't start the next collection until any requested activities from the last one are done.

[Saam Barati]

To solve the issue with the type profiler, I'm going to treat the type profiler log as a root to the GC. I think this issue is orthogonal to Fil's proposal for a more unified  and clean mechanism for asking the mutator to run certain code.

[Saam Barati]

Created attachment 294122 [details]
patch

[Saam Barati]

Comment on attachment 294122 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294122&action=review

> JSTests/typeProfiler/type-profiler-gc.js:3
> +// The goal of this test is to just ensure that we don't crash the type profiler.

This test actually doesn't crash on ToT, so I'll need to keep trying to write a test that exercises the fix.

[Geoffrey Garen]

Comment on attachment 294122 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294122&action=review

r=me

> Source/JavaScriptCore/runtime/TypeProfilerLog.cpp:107
> +    LogEntry* entry = m_logStartPtr;
> +    while (entry != m_currentLogEntryPtr) {

Since this loop has a beginning, a condition, and a step, I think it would be clearer as a for loop.

[Saam Barati]

Comment on attachment 294122 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294122&action=review

>> Source/JavaScriptCore/runtime/TypeProfilerLog.cpp:107
>> +    while (entry != m_currentLogEntryPtr) {
> 
> Since this loop has a beginning, a condition, and a step, I think it would be clearer as a for loop.

Sounds good to me. I'll make that change.

[Saam Barati]

Created attachment 294261 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 294261 [details]
patch for landing

Clearing flags on attachment: 294261

Committed r208483: <http://trac.webkit.org/changeset/208483>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.