[webkit-reviews] review granted: [Bug 194634] [JSC] Should have default NativeJITCode : [Attachment 362006] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 14 09:36:47 PST 2019

Mark Lam <mark.lam at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 194634: [JSC] Should have default NativeJITCode

Attachment 362006: Patch


--- Comment #5 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 362006
  --> https://bugs.webkit.org/attachment.cgi?id=362006

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

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

More information about the webkit-reviews mailing list