Bug 219830 – Move some of the work from JSLock to VMEntryScope

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

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 416111 [details]
Patch

[Mark Lam]

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.

[Tadeu Zagallo]

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.

[Mark Lam]

(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.

[Tadeu Zagallo]

Created attachment 416129 [details]
Patch

[Mark Lam]

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.

[Saam Barati]

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!

[Tadeu Zagallo]

Created attachment 416173 [details]
Patch for landing

[EWS]

Committed r270794: <https://trac.webkit.org/changeset/270794>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 416173 [details].

[Radar WebKit Bug Importer]

<rdar://problem/72307478>