Bug 202340 - [JSC] Consider replacing ExecState with JSGlobalObject
Summary: [JSC] Consider replacing ExecState with JSGlobalObject
Status: RESOLVED DUPLICATE of bug 202392
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-09-27 17:57 PDT by Yusuke Suzuki
Modified: 2019-11-20 01:14 PST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-09-27 17:57:18 PDT
...
Comment 1 Yusuke Suzuki 2019-09-27 17:59:57 PDT
After considering and discussing with Saam, I think this is a bit tricky. For host functions, we first need to get JSGlobalObject* to continue executing since many C++ function requires it. But getting JSGlobalObject* is not cheap.

1. Load Callee from ExecState
2. Load StructureID from Callee
3. Load Structure from StructureIDTable
4. Load GlobalObject from Structure

Previously we defer this lookup until we actually need it. We just pass around ExecState*. And we call ExecState::lexicalGlobalObject only when we really need it.
But if we pass around JSGlobalObject*, we first need to get it and I think this makes performance worse than now.
Comment 2 Yusuke Suzuki 2019-09-27 18:05:57 PDT
(In reply to Yusuke Suzuki from comment #1)
> After considering and discussing with Saam, I think this is a bit tricky.
> For host functions, we first need to get JSGlobalObject* to continue
> executing since many C++ function requires it. But getting JSGlobalObject*
> is not cheap.
> 
> 1. Load Callee from ExecState
> 2. Load StructureID from Callee
> 3. Load Structure from StructureIDTable
> 4. Load GlobalObject from Structure
> 
> Previously we defer this lookup until we actually need it. We just pass
> around ExecState*. And we call ExecState::lexicalGlobalObject only when we
> really need it.
> But if we pass around JSGlobalObject*, we first need to get it and I think
> this makes performance worse than now.

Another way is passing ExecState* and JSGlobalObject* to host native function. We can achieve that by embedding JSGlobalObject* pointer into native-function-call trampoline.
But this causes another problem. Currently, we have stub directory in VM for this trampoline code. So only one code per VM per function is generated. And since # of functions are statically bound, these trampoline code increase only when we create multiple VMs. And even more, once global GC is done, we can share them per process.

On the other hand, if we embed JSGlobalObject* to this trampoline, we need to create a trampoline per JSGlobalObject. If we create multiple frames, or navigates, then we get more and more trampolines. This trampoline code is a bit critical JIT code. We must succeed to generate this JIT code, otherwise, we cannot call this native host function. This is large difference from the usual JS JIT code, its generation can fail, and fall back to interpreter. Given that, I don't think creating trampolines per JSGlobalObject is good direction.
Comment 3 Yusuke Suzuki 2019-09-27 18:07:35 PDT
I'm now rather considering about just using the current way. We ensure that Callee is always small cell. And keep this invariant.
When creating IsoSubspace, pass classInfo, and check this does not implement their own getCallData/getConstructData. If it is implemented, we can just disable sharing tier for this type. Then, we can keep things simple while introducing sharing tier.
Comment 4 Yusuke Suzuki 2019-09-27 19:38:43 PDT
(In reply to Yusuke Suzuki from comment #3)
> I'm now rather considering about just using the current way. We ensure that
> Callee is always small cell. And keep this invariant.
> When creating IsoSubspace, pass classInfo, and check this does not implement
> their own getCallData/getConstructData. If it is implemented, we can just
> disable sharing tier for this type. Then, we can keep things simple while
> introducing sharing tier.

Talked with Saam. Another solution is putting VM& to TLS. A/B test for `ExecState::vm()` for LargeAllocation is telling that it may be possible that it has perf regression in Speedometer2 (0.54% with 98% probability). We could try TLS way instead.
Comment 5 Yusuke Suzuki 2019-11-20 01:14:18 PST

*** This bug has been marked as a duplicate of bug 202392 ***