1364520 - Remove the JSContext::jitTop optimization

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: jittop.patch

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

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: remove-jit-top.patch

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.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2688b23e8e9a
Remove the jitTop optimization; r=jandem

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2688b23e8e9a