Bug 202391 - [JSC] Place VM* in TLS
Summary: [JSC] Place VM* in TLS
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-30 22:36 PDT by Yusuke Suzuki
Modified: 2019-10-04 11:16 PDT (History)
16 users (show)

See Also:


Attachments
Patch (9.23 KB, patch)
2019-09-30 22:36 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (9.27 KB, patch)
2019-09-30 22:39 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (9.27 KB, patch)
2019-09-30 22:40 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.20 KB, patch)
2019-09-30 22:43 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (12.77 KB, patch)
2019-10-01 13:53 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (14.14 KB, patch)
2019-10-01 13:59 PDT, Yusuke Suzuki
mark.lam: review+
Details | Formatted Diff | Diff
Patch for landing (14.26 KB, patch)
2019-10-01 14:29 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-09-30 22:36:17 PDT
[JSC] Place VM* in TLS
Comment 1 Yusuke Suzuki 2019-09-30 22:36:31 PDT
Created attachment 379876 [details]
Patch
Comment 2 Yusuke Suzuki 2019-09-30 22:39:31 PDT
Created attachment 379877 [details]
Patch
Comment 3 Yusuke Suzuki 2019-09-30 22:40:59 PDT
Created attachment 379878 [details]
Patch
Comment 4 Yusuke Suzuki 2019-09-30 22:43:56 PDT
Created attachment 379879 [details]
Patch
Comment 5 Mark Lam 2019-09-30 23:03:16 PDT
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?
Comment 6 Mark Lam 2019-09-30 23:05:51 PDT
Looks like IDBBindingUtilities.cpp is being naughty and not using the JSLockHolder.
Comment 7 Keith Miller 2019-10-01 07:33:36 PDT
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...
Comment 8 Yusuke Suzuki 2019-10-01 13:53:16 PDT
Created attachment 379946 [details]
Patch
Comment 9 Yusuke Suzuki 2019-10-01 13:56:04 PDT
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.
Comment 10 Mark Lam 2019-10-01 13:57:53 PDT
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.
Comment 11 Yusuke Suzuki 2019-10-01 13:59:14 PDT
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.
Comment 12 Yusuke Suzuki 2019-10-01 13:59:45 PDT
Created attachment 379947 [details]
Patch
Comment 13 Mark Lam 2019-10-01 14:16:08 PDT
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.
Comment 14 Yusuke Suzuki 2019-10-01 14:28:54 PDT
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.
Comment 15 Yusuke Suzuki 2019-10-01 14:29:32 PDT
Created attachment 379951 [details]
Patch for landing
Comment 16 Yusuke Suzuki 2019-10-01 15:21:46 PDT
Committed r250583: <https://trac.webkit.org/changeset/250583>
Comment 17 Radar WebKit Bug Importer 2019-10-01 15:22:18 PDT
<rdar://problem/55892096>
Comment 18 Alex Christensen 2019-10-01 19:51:58 PDT
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
Comment 19 Yusuke Suzuki 2019-10-01 20:29:31 PDT
(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.
Comment 20 Yusuke Suzuki 2019-10-01 20:30:01 PDT
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.
Comment 21 Yusuke Suzuki 2019-10-01 20:41:23 PDT
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.
Comment 22 Yusuke Suzuki 2019-10-01 20:42:38 PDT
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.
Comment 23 Yusuke Suzuki 2019-10-01 20:44:44 PDT
(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.
Comment 24 Yusuke Suzuki 2019-10-01 20:48:24 PDT
Committed r250594: <https://trac.webkit.org/changeset/250594>
Comment 25 Saam Barati 2019-10-02 18:02:09 PDT
LGTM too with your assertion fix.

Out of curiosity, if we make exec->vm() use this, do we get a speedup?
Comment 26 Saam Barati 2019-10-02 18:02:22 PDT
(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)
Comment 27 Truitt Savell 2019-10-04 08:34:11 PDT
Reverted r250594 for reason:

Broke multiple internal API tests

Committed r250724: <https://trac.webkit.org/changeset/250724>
Comment 28 Truitt Savell 2019-10-04 08:35:07 PDT
Reverted r250583 for reason:

Broke multiple internal API tests

Committed r250725: <https://trac.webkit.org/changeset/250725>
Comment 29 Yusuke Suzuki 2019-10-04 11:16:59 PDT
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.