Bug 199786 – Concurrent GC should not rely on current phase to determine if it's safe to steal conn

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

[Tadeu Zagallo]

<rdar://problem/52505197>

[Tadeu Zagallo]

Created attachment 374094 [details]
Patch

[Saam Barati]

Comment on attachment 374094 [details]
Patch

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

> Source/JavaScriptCore/heap/Heap.cpp:2124
> +    if ((m_lastServedTicket == m_lastGrantedTicket) && !m_collectorThreadIsRunning) {

don’t you still need to check m_currentPhase? Can’t that also be driven by the mutator and we me in the middle of a collection?

[Saam Barati]

Comment on attachment 374094 [details]
Patch

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

setting back to r? based on my questions

> Source/JavaScriptCore/heap/Heap.cpp:261
> +        m_heap.m_collectorThreadIsRunning = false;

Is this the correct place to set this to false? Why are we guaranteed to run poll() again?

[Tadeu Zagallo]

Comment on attachment 374094 [details]
Patch

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

>> Source/JavaScriptCore/heap/Heap.cpp:261
>> +        m_heap.m_collectorThreadIsRunning = false;
> 
> Is this the correct place to set this to false? Why are we guaranteed to run poll() again?

Yes, unless the thread stops. I missed that. I'll update the patch to implement threadIsStopping and set it to false there.

>> Source/JavaScriptCore/heap/Heap.cpp:2124
>> +    if ((m_lastServedTicket == m_lastGrantedTicket) && !m_collectorThreadIsRunning) {
> 
> don’t you still need to check m_currentPhase? Can’t that also be driven by the mutator and we me in the middle of a collection?

This is called from the mutator thread, right? I think it's correct, but hypothetically, even if the mutator was collecting, all this would do is set the conn bit, which should already be set when the mutator is collecting.

[Tadeu Zagallo]

Created attachment 374106 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 374106 [details]
Patch

Clearing flags on attachment: 374106

Committed r247426: <https://trac.webkit.org/changeset/247426>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yusuke Suzuki]

Nice!!