Bug 219830

Summary: Move some of the work from JSLock to VMEntryScope
Product: WebKit Reporter: Tadeu Zagallo <tzagallo>
Component: JavaScriptCoreAssignee: Tadeu Zagallo <tzagallo>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, ews-watchlist, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Tadeu Zagallo 2020-12-12 19:03:24 PST
...
Comment 1 Tadeu Zagallo 2020-12-12 19:11:24 PST
Created attachment 416111 [details]
Patch
Comment 2 Mark Lam 2020-12-12 21:58:41 PST
Comment on attachment 416111 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        - lastStackTop: used by the interpreter and JIT, so VMEntry is guaranteed to execute

I think this is incorrect.  VM::m_lastStackTop is only needed to sanitize the stack, and we can call sanitizeStackForVM() when allocating objects.  I think you can allocate cells via the API without entering the VM, and you don't need to enter the VM to do so.  For example, via the ObjC API's [JSWrapperMap jsWrapperForObject:inContext:].  For this reason, this should be set for the current thread on JSLock acquisition.

> Source/JavaScriptCore/ChangeLog:10
> +        - stackPointerAtVMEntry: This sets the stack limits, which are used by the interpreter and JIT. As the name suggests, itâs only used after VM entry.

Non-ascii character in "it's"

> Source/JavaScriptCore/ChangeLog:19
> +        call is adding the current thread to the heap as this requires acquiring two locks. We canât  move this to VMEntryScope

non-ascii char in "can't"

> Source/JavaScriptCore/ChangeLog:20
> +        since itâs possible to use the API and GC without ever entering the VM, which would result in the current thread's stack

non-ascii char in "it's"

> Source/JavaScriptCore/ChangeLog:21
> +        not being scanned. Instead, we just remember the last thread that acquired the lock and skip the call if weâre seeing the

non-ascii char in "we're"

> Source/JavaScriptCore/ChangeLog:33
> +        UPCOMING_API:       Baseline   Change

Can you add a small blurb about what the "UPCOMING API" is?

> Source/WTF/ChangeLog:9
> +        Add a unique ID to threads. This uid is used in JSLock to avoid unnecessary work when the
> +        same thread that last acquired the lock is reacquiring it.

While I personally also always wanted to do this, why can't we just use &Thread as the id (which is how we're doing this elsewhere in the code)?

That said, I do like that the ID is monotonically increasing though, which is a very nice property to have.  Addresses of Threads can be reused as old threads die.

> Source/JavaScriptCore/runtime/JSLock.cpp:-138
> -    m_vm->setLastStackTop(thread.savedLastStackTop());
> -    ASSERT(thread.stack().contains(m_vm->lastStackTop()));

I think this need to stay here as explained above in the ChangeLog.

> Source/JavaScriptCore/runtime/JSLock.cpp:144
> +        if (m_vm->heap.machineThreads().addCurrentThread()) {

Hmmm, this is not caused by your patch, but what happens if a thread dies before the VM does?  I don't see any code to remove the thread from the ThreadGroup.  So far, the SamplingProfiler appears to be the only client of this machineThreads().  Can you file a bug to track that we should remove the thread from the ThreadGroup if a thread dies?

> Source/JavaScriptCore/runtime/VMEntryScope.cpp:82
> +        Thread& thread = Thread::current();
> +        vm.setLastStackTop(thread.savedLastStackTop());
> +        ASSERT(thread.stack().contains(vm.lastStackTop()));
> +
> +        RELEASE_ASSERT(!vm.stackPointerAtVMEntry());
> +        void* p = currentStackPointer();
> +        vm.setStackPointerAtVMEntry(p);
> +
> +#if ENABLE(WEBASSEMBLY)
> +        if (Wasm::isSupported())
> +            Wasm::startTrackingCurrentThread();
> +#endif
> +
> +#if HAVE(MACH_EXCEPTIONS)
> +        registerThreadForMachExceptionHandling(Thread::current());
> +#endif
> +
> +        vm.firePrimitiveGigacageEnabledIfNecessary();

In the current code, all of these would have already been executed on JSLock acquisition before the above VMEntryScope code is executed.  That would argue for putting this higher up.  I suggest putting them right after `vm.entryScope = this;`.

Also, you already have the current thread.  You don't have to call Thread::current() again for registerThreadForMachExceptionHandling().

> Source/WTF/wtf/Threading.h:334
>      PlatformThreadHandle m_handle;

Can you put the m_uid here after m_handle instead?  There's currently 4 bytes of unused padding here but not above after m_mutex.
Comment 3 Tadeu Zagallo 2020-12-13 10:22:50 PST
Comment on attachment 416111 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:9
>> +        - lastStackTop: used by the interpreter and JIT, so VMEntry is guaranteed to execute
> 
> I think this is incorrect.  VM::m_lastStackTop is only needed to sanitize the stack, and we can call sanitizeStackForVM() when allocating objects.  I think you can allocate cells via the API without entering the VM, and you don't need to enter the VM to do so.  For example, via the ObjC API's [JSWrapperMap jsWrapperForObject:inContext:].  For this reason, this should be set for the current thread on JSLock acquisition.

Thanks, I missed that we called sanitizeStackForVM when allocating. I'll move it back.

>> Source/JavaScriptCore/ChangeLog:10
>> +        - stackPointerAtVMEntry: This sets the stack limits, which are used by the interpreter and JIT. As the name suggests, itâs only used after VM entry.
> 
> Non-ascii character in "it's"

Oops, I wrote it in Notes then copied it over here and didn't notice it.

>> Source/JavaScriptCore/ChangeLog:33
>> +        UPCOMING_API:       Baseline   Change
> 
> Can you add a small blurb about what the "UPCOMING API" is?

I described it in the patch that introduced the benchmark. Since this is just an improvement, I don't think this is the right place for it.

>> Source/WTF/ChangeLog:9
>> +        same thread that last acquired the lock is reacquiring it.
> 
> While I personally also always wanted to do this, why can't we just use &Thread as the id (which is how we're doing this elsewhere in the code)?
> 
> That said, I do like that the ID is monotonically increasing though, which is a very nice property to have.  Addresses of Threads can be reused as old threads die.

I was mostly just thinking about the case you mentioned where the address is reused. With AutomaticThreads or dispatch_queues it doesn't seem too unlikely that an address might be reused. Given that this is so simple to implement, I didn't think there was any downside to it.

>> Source/JavaScriptCore/runtime/JSLock.cpp:144
>> +        if (m_vm->heap.machineThreads().addCurrentThread()) {
> 
> Hmmm, this is not caused by your patch, but what happens if a thread dies before the VM does?  I don't see any code to remove the thread from the ThreadGroup.  So far, the SamplingProfiler appears to be the only client of this machineThreads().  Can you file a bug to track that we should remove the thread from the ThreadGroup if a thread dies?

The thread removes itself from all ThreadGroups in Thread::didExit.

>> Source/JavaScriptCore/runtime/VMEntryScope.cpp:82
>> +        vm.firePrimitiveGigacageEnabledIfNecessary();
> 
> In the current code, all of these would have already been executed on JSLock acquisition before the above VMEntryScope code is executed.  That would argue for putting this higher up.  I suggest putting them right after `vm.entryScope = this;`.
> 
> Also, you already have the current thread.  You don't have to call Thread::current() again for registerThreadForMachExceptionHandling().

Good call, will update both things.

>> Source/WTF/wtf/Threading.h:334
>>      PlatformThreadHandle m_handle;
> 
> Can you put the m_uid here after m_handle instead?  There's currently 4 bytes of unused padding here but not above after m_mutex.

will do.
Comment 4 Mark Lam 2020-12-13 11:12:01 PST
(In reply to Tadeu Zagallo from comment #3)
> >> Source/JavaScriptCore/runtime/JSLock.cpp:144
> >> +        if (m_vm->heap.machineThreads().addCurrentThread()) {
> > 
> > Hmmm, this is not caused by your patch, but what happens if a thread dies before the VM does?  I don't see any code to remove the thread from the ThreadGroup.  So far, the SamplingProfiler appears to be the only client of this machineThreads().  Can you file a bug to track that we should remove the thread from the ThreadGroup if a thread dies?
> 
> The thread removes itself from all ThreadGroups in Thread::didExit.

Ah, yes.  I missed that.
Comment 5 Tadeu Zagallo 2020-12-13 17:22:55 PST
Created attachment 416129 [details]
Patch
Comment 6 Mark Lam 2020-12-14 10:18:34 PST
Comment on attachment 416129 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/JSLock.h:148
> +    uint32_t m_lastOwnerThread { 0 };

nit: Not the most important thing but can you swap the position of this with m_shouldReleaseHeapAccess below.  That should pack the fields better.
Comment 7 Saam Barati 2020-12-14 10:42:15 PST
Comment on attachment 416129 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:40
> +        Score:             36.2211   43.3368

nice!
Comment 8 Tadeu Zagallo 2020-12-14 10:59:57 PST
Created attachment 416173 [details]
Patch for landing
Comment 9 EWS 2020-12-14 11:48:03 PST
Committed r270794: <https://trac.webkit.org/changeset/270794>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416173 [details].
Comment 10 Radar WebKit Bug Importer 2020-12-14 11:49:16 PST
<rdar://problem/72307478>