Closed Bug 1364520 Opened 3 years ago Closed 3 years ago

Remove the JSContext::jitTop optimization

Categories

(Core :: JavaScript Engine: JIT, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch jittop.patch (obsolete) — Splinter Review
This patch:
- removes jitTop from JSContext. Instead, each JitActivation has its jitTop value.
- renames jitTop to exitFP: it's the same naming we're using in wasm, and it seems more talkative to me: it's just the frame pointer on exit. I've been very confused by jitTop: top of what? how do we know it's a frame pointer until we read code that uses it?

The following micro-benchmark, just calling a non-inlined C++ function, shows some very low improvement: from ~550ms to ~525ms, but I assume this is noise.

var t = dateNow();
for (var i = 100000000; i --> 0 ;) {
    Math.toSource();
}
print(dateNow() - t);

It's a nice simplification and helps us for merging wasm::Activation into JitActivation (viz. we won't have to handle prevJitTop anymore in wasm).
Attachment #8867296 - Flags: review?(jdemooij)
Comment on attachment 8867296 [details] [diff] [review]
jittop.patch

Review of attachment 8867296 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, this is definitely easier to reason about.

::: js/src/vm/Stack.cpp
@@ +1724,5 @@
>  
> +uint8_t*
> +Activation::exitFP() const
> +{
> +    return isJit() ? asJit()->exitFP() : nullptr;

I think it would be nicer to have exitFP() only on JitActivation, even if it requires some isJit()/asJit() calls in the callers. exitFP is a JIT implementation detail that Activation doesn't need to know about, and it doesn't make sense to call exitFP() when we have, say, an InterpreterActivation.

::: js/src/vm/Stack.h
@@ +1477,5 @@
>      }
>      Activation* activation() const {
>          return activation_;
>      }
> +    uint8_t* exitFP() const {

Similar to the other comment, I'd prefer removing this or moving it to JitActivationIterator. Before this patch, ActivationIterator had to know about jitTop because it had to update it, but now that that's gone it seems cleaner to remove this and require an explicit asJit() call.
Attachment #8867296 - Flags: review?(jdemooij) → review+
carrying forward r+; a fix was needed for GetTopProfilingJitFrame, which has been made static (it was exported before, but never used outside vm/GeckoProfiler.cpp), and adapter to take a js::Activation as a parameter.
Attachment #8867296 - Attachment is obsolete: true
Attachment #8874435 - Flags: review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2688b23e8e9a
Remove the jitTop optimization; r=jandem
https://hg.mozilla.org/mozilla-central/rev/2688b23e8e9a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.