WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
194634
[JSC] Should have default NativeJITCode
https://bugs.webkit.org/show_bug.cgi?id=194634
Summary
[JSC] Should have default NativeJITCode
Yusuke Suzuki
Reported
2019-02-13 19:15:42 PST
We are creating so many NativeJITCode which are identical... Just calling trampoline.
Attachments
Patch
(3.34 KB, patch)
2019-02-14 02:02 PST
,
Yusuke Suzuki
mark.lam
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2019-02-13 19:16:00 PST
It takes 14KB in JSGlobalObject initialization.
Yusuke Suzuki
Comment 2
2019-02-14 02:02:15 PST
Created
attachment 362006
[details]
Patch
Caio Lima
Comment 3
2019-02-14 03:00:41 PST
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); ... ```
Caio Lima
Comment 4
2019-02-14 03:35:57 PST
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.
Mark Lam
Comment 5
2019-02-14 09:36:47 PST
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.
Caio Lima
Comment 6
2019-02-14 10:50:14 PST
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?
Mark Lam
Comment 7
2019-02-14 10:52:13 PST
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.
Yusuke Suzuki
Comment 8
2019-02-14 10:55:29 PST
Comment on
attachment 362006
[details]
Patch Thank you for your review!
Yusuke Suzuki
Comment 9
2019-02-14 11:10:32 PST
Committed
r241557
: <
https://trac.webkit.org/changeset/241557
>
Radar WebKit Bug Importer
Comment 10
2019-02-14 11:14:20 PST
<
rdar://problem/48080631
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug