Bug 194634 – [JSC] Should have default NativeJITCode

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

[Yusuke Suzuki]

We are creating so many NativeJITCode which are identical... Just calling trampoline.

[Yusuke Suzuki]

It takes 14KB in JSGlobalObject initialization.

[Yusuke Suzuki]

Created attachment 362006 [details]
Patch

[Caio Lima]

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 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 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 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 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 on attachment 362006 [details]
Patch

Thank you for your review!

[Yusuke Suzuki]

Committed r241557: <https://trac.webkit.org/changeset/241557>

[Radar WebKit Bug Importer]

<rdar://problem/48080631>