Closed Bug 1417555 Opened 2 years ago Closed 2 years ago

Simplify offsetOfNativeOrScript and how jit code is loaded


(Core :: JavaScript Engine: JIT, enhancement, P3)




Tracking Status
firefox59 --- fixed


(Reporter: bbouvier, Assigned: bbouvier)




(4 files)

It appears we're committing a crime of unnecessary over-generality here, and offsetOfNativeOrScript could probably be split into two methods offsetOfNative and offsetOfScript, because the callers ~always [1] know what they expect.

For bug 1319203, we want to introduce a new struct in the JSFunction union U: a WasmNative, which is a native function **and** a jit entry. The idea being that we could reuse the loadBaselineOrIonRaw / load-at-offset pattern to load the wasm jit-entry. So for this, we'd want the WasmNative's native to be located at the same place as a regular native in the union, and the WasmNative's jit entry to be located at the same spot as the scripted function's script. Which motivates the following shuffling: in the union, put scripted's env first and then the other sub-struct s.

[1] it's not always the case: sometimes we want to make sure a given callee is an expected native, and we're not sure the function is scripted or native. In this case, we'd compare either a Native or the scripted's s.env (because of the shuffling), and this sounds fine, because both are memory addresses, so there can not be false positives.
Blocks: 1319203
Attached patch 1.random.patchSplinter Review
Random cleanups.
Attachment #8928619 - Flags: review?(jdemooij)
Attachment #8928620 - Flags: review?(jdemooij)
Attachment #8928621 - Flags: review?(jdemooij)
Attachment #8928622 - Flags: review?(jdemooij)
Attachment #8928619 - Flags: review?(jdemooij) → review+
Attachment #8928620 - Flags: review?(jdemooij) → review+
Comment on attachment 8928621 [details] [diff] [review]

Review of attachment 8928621 [details] [diff] [review]:

This is great, it also makes the code more readable.

::: js/src/jit/BaselineIC.cpp
@@ +3842,5 @@
>      masm.Push(scratch);
>      // Load nargs into scratch for underflow check, and then load jitcode pointer into target.
>      masm.load16ZeroExtend(Address(target, JSFunction::offsetOfNargs()), scratch);
> +    masm.loadPtr(Address(target, JSFunction::offsetOfScript()), target);

So we called guardFunApply to guard the callee is scripted. Could you remove the checkNative argument from guardFunApply while you're here? Both callers always pass |false| and this confused me for a bit.

::: js/src/jit/CodeGenerator.cpp
@@ +2796,2 @@
>      masm.store32(Imm32(u.word), Address(output, JSFunction::offsetOfNargs()));
> +    masm.storePtr(ImmGCPtr(info.scriptOrLazyScript), Address(output, JSFunction::offsetOfScript()));

For this one it would be nice to add JSFunction::offsetOfScriptOrLazyScript, similar to offsetOfNativeOrEnv you added. I don't expect that to change anytime soon, but it's nice to document/check the invariant.
Attachment #8928621 - Flags: review?(jdemooij) → review+
Comment on attachment 8928622 [details] [diff] [review]

Review of attachment 8928622 [details] [diff] [review]:

Nice cleanup.
Attachment #8928622 - Flags: review?(jdemooij) → review+
Pushed by
Random cleanups; r=jandem
Use more descriptive names in the JSFunction union; r=jandem
Rejigger how native/script are stored in JSFunction and update callers; r=jandem
Fuse the script load and jit code load into one masm instruction; r=jandem
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.