Bug 202391 – [JSC] Place VM* in TLS

• Component: New Bugs
• Status: REOPENED

[Yusuke Suzuki]

[JSC] Place VM* in TLS

[Yusuke Suzuki]

Created attachment 379876 [details]
Patch

[Yusuke Suzuki]

Created attachment 379877 [details]
Patch

[Yusuke Suzuki]

Created attachment 379878 [details]
Patch

[Yusuke Suzuki]

Created attachment 379879 [details]
Patch

[Mark Lam]

Comment on attachment 379879 [details]
Patch

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

LGTM

> Source/JavaScriptCore/runtime/JSLock.cpp:293
> +    VM::exchange(nullptr);

Maybe assert that the returned VM* == m_vm?

> Source/JavaScriptCore/runtime/JSLock.cpp:311
> +    VM::exchange(m_vm.get());

Assert that VM::exchange returns nullptr?

> Source/JavaScriptCore/runtime/JSLock.h:91
> +    friend class JSRunLoopTimer;

Does JSRunLoopTimer really need to be a friend?

[Mark Lam]

Looks like IDBBindingUtilities.cpp is being naughty and not using the JSLockHolder.

[Keith Miller]

Comment on attachment 379879 [details]
Patch

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

> Source/JavaScriptCore/runtime/VM.h:318
> +    static constexpr pthread_key_t tlsKey = WTF_GC_TLC_KEY;

Can we change this name? WTF_GC_TLC_KEY doesn't seem super accurate...

[Yusuke Suzuki]

Created attachment 379946 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 379879 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSLock.cpp:293
>> +    VM::exchange(nullptr);
> 
> Maybe assert that the returned VM* == m_vm?

Fixed.

>> Source/JavaScriptCore/runtime/JSLock.cpp:311
>> +    VM::exchange(m_vm.get());
> 
> Assert that VM::exchange returns nullptr?

Fixed.

>> Source/JavaScriptCore/runtime/JSLock.h:91
>> +    friend class JSRunLoopTimer;
> 
> Does JSRunLoopTimer really need to be a friend?

Not necessary. Removed, nice.

>> Source/JavaScriptCore/runtime/VM.h:318
>> +    static constexpr pthread_key_t tlsKey = WTF_GC_TLC_KEY;
> 
> Can we change this name? WTF_GC_TLC_KEY doesn't seem super accurate...

Changed it to WTF_VM_KEY. Thanks.

[Mark Lam]

Comment on attachment 379946 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSLock.cpp:90
> +        VM::exchange(m_previousVMInTLS); // Change VM after unlocking. Otherwise, willReleaseLock will see previous VM.

I suggest rephrasing this as "Change VM only after unlocking".  I think that's what you meant.

[Yusuke Suzuki]

Comment on attachment 379946 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSLock.cpp:90
>> +        VM::exchange(m_previousVMInTLS); // Change VM after unlocking. Otherwise, willReleaseLock will see previous VM.
> 
> I suggest rephrasing this as "Change VM only after unlocking".  I think that's what you meant.

Fixed, thanks.

[Yusuke Suzuki]

Created attachment 379947 [details]
Patch

[Mark Lam]

Comment on attachment 379947 [details]
Patch

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

r=me with the ChangeLog fixed.

> Source/JavaScriptCore/ChangeLog:10
> +        And JSLockHolder's destructor restores it. Since DropAllLocks want to put `nullptr` in TLS, we place previous VM*
> +        in JSLockHolder itself. And now JSLock is only lockable by JSLockHolder.

This part "Since DropAllLocks want to put `nullptr` in TLS, we place previous VM* in JSLockHolder itself." is not true.  DropAllLocks knows the VM that it is dropping the lock for, and it doesn't interact with JSLockHolder, right?  So, JSLockHolder::m_previousVMInTLS is not needed for this purpose.

That said, let's say that we have 2 VM instances A and B, and that we were executing in A when it called a C++ host function.  While still holding the JSLock for A, the thread now enters B.  In this case, the TLS VM should be set to B, but when we exit, it should be restored to A.  I think this is the reason for needing JSLockHolder::m_previousVMInTLS.  This is also the reason why we should not generally use VM::current() as the source of truth of VM* rather than threading everywhere: it is only correct for some uses.

[Yusuke Suzuki]

Comment on attachment 379947 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/ChangeLog:10
>> +        in JSLockHolder itself. And now JSLock is only lockable by JSLockHolder.
> 
> This part "Since DropAllLocks want to put `nullptr` in TLS, we place previous VM* in JSLockHolder itself." is not true.  DropAllLocks knows the VM that it is dropping the lock for, and it doesn't interact with JSLockHolder, right?  So, JSLockHolder::m_previousVMInTLS is not needed for this purpose.
> 
> That said, let's say that we have 2 VM instances A and B, and that we were executing in A when it called a C++ host function.  While still holding the JSLock for A, the thread now enters B.  In this case, the TLS VM should be set to B, but when we exit, it should be restored to A.  I think this is the reason for needing JSLockHolder::m_previousVMInTLS.  This is also the reason why we should not generally use VM::current() as the source of truth of VM* rather than threading everywhere: it is only correct for some uses.

OK, fixed.

[Yusuke Suzuki]

Created attachment 379951 [details]
Patch for landing

[Yusuke Suzuki]

Committed r250583: <https://trac.webkit.org/changeset/250583>

[Radar WebKit Bug Importer]

<rdar://problem/55892096>

[Alex Christensen]

Comment on attachment 379951 [details]
Patch for landing

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

> Source/JavaScriptCore/runtime/JSLock.cpp:313
> +    ASSERT_UNUSED(previousVMInTLS, previousVMInTLS);

This seems to assert in many debug tests
https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20(Tests)/builds/5020/steps/layout-test/logs/stdio

[Yusuke Suzuki]

(In reply to Alex Christensen from comment #18)
> Comment on attachment 379951 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379951&action=review
> 
> > Source/JavaScriptCore/runtime/JSLock.cpp:313
> > +    ASSERT_UNUSED(previousVMInTLS, previousVMInTLS);
> 
> This seems to assert in many debug tests
> https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20(Tests)/
> builds/5020/steps/layout-test/logs/stdio

Looking.

[Yusuke Suzuki]

Comment on attachment 379951 [details]
Patch for landing

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

>>> Source/JavaScriptCore/runtime/JSLock.cpp:313
>>> +    ASSERT_UNUSED(previousVMInTLS, previousVMInTLS);
>> 
>> This seems to assert in many debug tests
>> https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK2%20(Tests)/builds/5020/steps/layout-test/logs/stdio
> 
> Looking.

This assertion should not be met. Removing.

[Yusuke Suzuki]

Comment on attachment 379951 [details]
Patch for landing

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

> Source/JavaScriptCore/runtime/JSLock.cpp:294
> +    ASSERT_UNUSED(previousVMInTLS, previousVMInTLS == m_vm.get());

I don't think this is guaranteed. At anytime, we can use DropAllLocks.

[Yusuke Suzuki]

Comment on attachment 379951 [details]
Patch for landing

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

>> Source/JavaScriptCore/runtime/JSLock.cpp:294
>> +    ASSERT_UNUSED(previousVMInTLS, previousVMInTLS == m_vm.get());
> 
> I don't think this is guaranteed. At anytime, we can use DropAllLocks.

We should put VM* in m_previousVMInTLS field.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #22)
> Comment on attachment 379951 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=379951&action=review
> 
> >> Source/JavaScriptCore/runtime/JSLock.cpp:294
> >> +    ASSERT_UNUSED(previousVMInTLS, previousVMInTLS == m_vm.get());
> > 
> > I don't think this is guaranteed. At anytime, we can use DropAllLocks.
> 
> We should put VM* in m_previousVMInTLS field.

Ah, no. After looking into DropAllLocks, it strongly assumes that VM is now used.

[Yusuke Suzuki]

Committed r250594: <https://trac.webkit.org/changeset/250594>

[Saam Barati]

LGTM too with your assertion fix.

Out of curiosity, if we make exec->vm() use this, do we get a speedup?

[Saam Barati]

(In reply to Saam Barati from comment #25)
> LGTM too with your assertion fix.
> 
> Out of curiosity, if we make exec->vm() use this, do we get a speedup?

(When FAST_TLS is enabled)

[Truitt Savell]

Reverted r250594 for reason:

Broke multiple internal API tests

Committed r250724: <https://trac.webkit.org/changeset/250724>

[Truitt Savell]

Reverted r250583 for reason:

Broke multiple internal API tests

Committed r250725: <https://trac.webkit.org/changeset/250725>

[Yusuke Suzuki]

Comment on attachment 379951 [details]
Patch for landing

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

>>>> Source/JavaScriptCore/runtime/JSLock.cpp:294
>>>> +    ASSERT_UNUSED(previousVMInTLS, previousVMInTLS == m_vm.get());
>>> 
>>> I don't think this is guaranteed. At anytime, we can use DropAllLocks.
>> 
>> We should put VM* in m_previousVMInTLS field.
> 
> Ah, no. After looking into DropAllLocks, it strongly assumes that VM is now used.

WebThread clients uses DropAllLocks even if they do not have a lock for VM. We should check m_droppedLockCount.