We are creating so many NativeJITCode which are identical... Just calling trampoline.
It takes 14KB in JSGlobalObject initialization.
Created attachment 362006 [details] Patch
Comment on attachment 362006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362006&action=review Informal review here. Patch LGTM, but I have some questions. > Source/JavaScriptCore/runtime/VM.cpp:693 > + static NativeJITCode* result; It is a little nit picking, but maybe would be good initialise this to nullptr. > Source/JavaScriptCore/runtime/VM.cpp:695 > + std::call_once(onceKey, [&] { I was wondering why do you prefer this instead of: ``` if (!result) result = new NativeJITCode(LLInt::getCodeRef<JSEntryPtrTag>(llint_native_call_trampoline), JITCode::HostCallThunk, NoIntrinsic); return makeRef(*result); ... ```
Comment on attachment 362006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362006&action=review > Source/JavaScriptCore/runtime/VM.cpp:698 > + return makeRef(*result); IIUC, NativeJITCode is ThreadSafeRefCounted. How do we guarantee that ```result``` will always pointing to a valid object? Once last ```NativeExecutable``` is destroyed, it will also destroy NativeJITCode, but ```result``` will be pointing to an invalid address.
Comment on attachment 362006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362006&action=review r=me with ref fix. >> Source/JavaScriptCore/runtime/VM.cpp:693 >> + static NativeJITCode* result; > > It is a little nit picking, but maybe would be good initialise this to nullptr. I think statics already null by default. >> Source/JavaScriptCore/runtime/VM.cpp:695 >> + std::call_once(onceKey, [&] { > > I was wondering why do you prefer this instead of: > > ``` > if (!result) > result = new NativeJITCode(LLInt::getCodeRef<JSEntryPtrTag>(llint_native_call_trampoline), JITCode::HostCallThunk, NoIntrinsic); > > return makeRef(*result); > ... > ``` A process may instantiate more than 1 VMs on multiple threads. Hence, std::call_once is necessary. >> Source/JavaScriptCore/runtime/VM.cpp:698 >> + return makeRef(*result); > > IIUC, NativeJITCode is ThreadSafeRefCounted. How do we guarantee that ```result``` will always pointing to a valid object? Once last ```NativeExecutable``` is destroyed, it will also destroy NativeJITCode, but ```result``` will be pointing to an invalid address. I think Caio is correct here. This can happen if the VM is destroyed before we create any new VMs. Let's ref the NativeJITCode inside the std::call_once so that it will never be deref'ed to 0.
Comment on attachment 362006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362006&action=review >>> Source/JavaScriptCore/runtime/VM.cpp:695 >>> + std::call_once(onceKey, [&] { >> >> I was wondering why do you prefer this instead of: >> >> ``` >> if (!result) >> result = new NativeJITCode(LLInt::getCodeRef<JSEntryPtrTag>(llint_native_call_trampoline), JITCode::HostCallThunk, NoIntrinsic); >> >> return makeRef(*result); >> ... >> ``` > > A process may instantiate more than 1 VMs on multiple threads. Hence, std::call_once is necessary. Thx for the explanation! >>> Source/JavaScriptCore/runtime/VM.cpp:698 >>> + return makeRef(*result); >> >> IIUC, NativeJITCode is ThreadSafeRefCounted. How do we guarantee that ```result``` will always pointing to a valid object? Once last ```NativeExecutable``` is destroyed, it will also destroy NativeJITCode, but ```result``` will be pointing to an invalid address. > > I think Caio is correct here. This can happen if the VM is destroyed before we create any new VMs. Let's ref the NativeJITCode inside the std::call_once so that it will never be deref'ed to 0. Is it possible to write a test to cover such case?
Comment on attachment 362006 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362006&action=review >>> Source/JavaScriptCore/runtime/VM.cpp:698 >>> + return makeRef(*result); >> >> IIUC, NativeJITCode is ThreadSafeRefCounted. How do we guarantee that ```result``` will always pointing to a valid object? Once last ```NativeExecutable``` is destroyed, it will also destroy NativeJITCode, but ```result``` will be pointing to an invalid address. > > I think Caio is correct here. This can happen if the VM is destroyed before we create any new VMs. Let's ref the NativeJITCode inside the std::call_once so that it will never be deref'ed to 0. Yusuke pointed out that this is already handled by makeRef(). Note: he's not using adoptRef(). So all is well.
Comment on attachment 362006 [details] Patch Thank you for your review!
Committed r241557: <https://trac.webkit.org/changeset/241557>
<rdar://problem/48080631>