1344477 - Add dedicated opcode for function call ignoring return value

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Tooru Fujisawa [:arai]]

separated from bug 1342070.

there would be several native functions that can be optimized when return value is known to be ignored.
we could emit dedicated opcode for that case, and call different native function.

there's already optimized case: Array#splice.

we could generalize it, and support more functions.

my plan is:
  * emit dedicated opcode (JSOP_NORVCALL ?) when the return value is ignored
    (perhaps we could just check next opcode every time, instead of using dedicated opcode?)
  * use JitInfo to define another native function for the case return value is ignored
  * call the dedicated native function from JSOP_NORVCALL
  * also support it in JIT

I'll apply it to Array#splice here.

Comment 1

[Tooru Fujisawa [:arai]]

with dedicated opcode, we can optimize some more case than JSOP_CALL + JSOP_POP,
like `a ? f() : g()` case etc.
(of course we could trace bytecode, but I don't think it makes sense to check it everytime

Comment 2

[Tooru Fujisawa [:arai]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=689ccb1d8daa31872f1839b1c00d798f5dad06df
supported in JIT.

haven't yet applied to `a ? f() : g()` case.

Comment 3

[Tooru Fujisawa [:arai]]

another try https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e10bf8095925de06a4436e94211502aea24fca0
fixed baseline call return address and support `a ? f() : g()`, and also some more.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Performance comparison with splice

:)

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add JSOP_NORVCALL for function call that ignores return value.

Added JSOP_NORVCALL (NO R[eturn] V[alue] CALL, if you know better name, let me know), that's the variant of JSOP_CALL.

It's emitted in the following places:
  f1();
  f2(), f3(), f4();
  RequireObjectCoercible in self-hosted JS
  for (f5() ; ... ; f6()) {}
  ... ? f7() : f8();

  f1, f4, f7, f8 : only in non-eval context
  others:          always

Also added NoRetValNative type to JSJitInfo, that holds JSNative that can be called when the return value is ignored.
A native function that can be optimized in that case can now have JSJitInfo with that type, like:

> const JSJitInfo js::array_splice_info = {
>   { (JSJitGetterOp)array_splice_noRetVal },
>   { 0 }, /* unused */
>   { 0 }, /* unused */
>   JSJitInfo::NoRetValNative,
>   JSJitInfo::AliasEverything,
>   JSVAL_TYPE_UNDEFINED,
> };
> ...
>     JS_FNINFO("splice",         array_splice,       &array_splice_info, 2,0),

Also, added noRetVal flag to CallArgs, that is set for JSOP_NORVCALL.

InternalCallOrConstruct now checks if the target is native function and it has JSJitInfo with NoRetValNative type, and in that case calls it instead of fun->native(), if CallArgs.noRetVal is set.

In Interpreter,
JSOP_NORVCALL just uses InternalCallOrConstruct via CallFromStack.

In Baseline,
noRetVal flag is added to DoCallFallback and ICCall_Native.
DoCallFallback uses InternalCallOrConstruct via CallFromStack.

ICCall_Native is a bit tricky, since its noRetVal flag is used only when the opcode is JSOP_NORVCALL and the target function has JSJitInfo with NoRetValNative type.  (instead of all cases when the opcode is JSOP_NORVCALL)
that's because JSJitInfo.type_ is a bit field, and I cannot access it from JIT code.
So, instead of checking type_ in JIT code, I applied ICCall_Native+noRetVal==true optimization only for native function with JSJitInfo with NoRetValNative case.
Otherwise, ICCall_Native+noRetVal==false IC is attached.

In Ion,
visitCallNative uses the JSJitInfo when the opcode is JSOP_NORVCALL and the target function has JSJitInfo with NoRetValNative.

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Optimize Array.prototype.splice with JSOP_NORVCALL.

Applied the optimization for Array.prototype.splice, removing dedicated IRs.
Now array_splice_impl+returnValueIsUsed==false path is used also in Interpreter and Baseline execution.

here's benchmark result

Octane: http://bnjbvr.github.io/simplegraph/#title=Octane%20result%20comparison&ytitle=Values&categories=Richards%2CDeltaBlue%2CCrypto%2CRayTrace%2CEarleyBoyer%2CRegExp%2CSplay%2CSplayLatency%2CNavierStokes%2CPdfJS%2CMandreel%2CMandreelLatency%2CGameboy%2CCodeLoad%2CBox2D%2Czlib%2CTypescript%2CScore%20(version%209)&values=before%2028105%2056292%2026392%2093218%2024868%203866%2016236%2018926%2038666%2011782%2026523%2025461%2043721%2015639%2046283%2071156%2027130%2026989%0Aafter%2028452%2055948%2026358%2094613%2024938%203871%2016438%2018699%2038635%2012223%2026470%2025572%2045317%2015513%2047895%2071307%2027005%2027188
Kraken: http://bnjbvr.github.io/simplegraph/#title=Kraken%20result%20comparison&ytitle=Values&categories=audio-fft%2Cstanford-crypto-pbkdf2%2Caudio-beat-detection%2Cstanford-crypto-ccm%2Cimaging-darkroom%2Cjson-parse-financial%2Caudio-oscillator%2Cai-astar%2Caudio-dft%2Cstanford-crypto-sha256-iterative%2Cjson-stringify-tinderbox%2Cimaging-gaussian-blur%2Cstanford-crypto-aes%2Cimaging-desaturate&values=before%2051.8%20108.4%2089.3%2089.3%2075.5%2043.2%2062.8%2075.8%20124.3%2042%2040.3%2072%2058%2063%0Aafter%2052%20110.1%2089.2%2087.8%2076%2042.1%2062.5%2075.5%20124.7%2042%2040.8%2072.7%2057.8%2063.8
SunSpider: http://bnjbvr.github.io/simplegraph/#title=SunSpider%20result%20comparison&ytitle=Values&categories=math-spectral-norm%2Cbitops-3bit-bits-in-byte%2Cdate-format-xparb%2C3d-morph%2Ccontrolflow-recursive%2Cstring-base64%2C3d-raytrace%2Caccess-nsieve%2Ccrypto-md5%2Cbitops-nsieve-bits%2Caccess-binary-trees%2Cbitops-bitwise-and%2Cstring-validate-input%2Caccess-fannkuch%2Cstring-fasta%2Cregexp-dna%2Cmath-partial-sums%2Caccess-nbody%2Cstring-tagcloud%2Cdate-format-tofte%2Cstring-unpack-code%2Ccrypto-aes%2Ccrypto-sha1%2Cbitops-bits-in-byte%2Cmath-cordic%2C3d-cube&values=before%201.9%201.1%2014.7%205%202.3%206.3%2012%202.9%204.1%203.8%202.4%201.9%208.7%207%207.6%2015.3%206.7%202.8%2021.5%2010.5%2038.4%2013.3%203.9%202.4%202.6%2013.9%0Aafter%201.7%201%2014.5%204.9%202.4%206.2%2011.9%202.9%203.9%203.7%202.5%201.9%208.7%207%207.5%2015.1%206.4%202.9%2021.4%2010.5%2037.9%2012.9%203.8%202.3%202.5%2013.7

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8843656 [details] [diff] [review]
Part 1: Add JSOP_NORVCALL for function call that ignores return value.

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

Looks great! Below are some suggestions to simplify the patch a bit, but the approach is fine.

::: js/public/CallArgs.h
@@ +128,5 @@
>    protected:
>      Value* argv_;
>      unsigned argc_;
> +    bool constructing_:1;
> +    bool noRetVal_:1;

What do you think about ignoresReturnValue_ or ignoredReturnValue_? And maybe JSOP_CALL_IGNORE{D,S}_RV? That would also match IgnoreValue in the emitter. Also please add a brief comment like // True if the caller does not use the return value.

I think it's a little clearer than 'no return value', because there usually *is* a return value, we're just ignoring it :) (I was also considering unusedReturnValue_, but that's confusing because of the "used rval" code in this file.)

::: js/src/frontend/BytecodeEmitter.h
@@ +166,5 @@
>      // clobbers the list.
>      void patchAll(jsbytecode* code, JumpTarget target);
>  };
>  
> +enum ValueUsage {

Nit: make this an enum class. A little more verbose but the improved type safety is worth it.

::: js/src/jit/BaselineIC.h
@@ +759,5 @@
> +            (static_cast<int32_t>(Engine::Baseline)) |
> +            (static_cast<int32_t>(ICStub::Call_Fallback) << 1) |
> +            (0 << 17) | // spread
> +            (0 << 18) | // constructing
> +            (1 << 19);  // noRetVal

I think we don't need to add noRetVal to the fallback stub as its JIT code is the same for both cases. Then you also don't need to change baselineCallReturnAddrs_.

(We really need to convert the Call IC code to CacheIR so we don't need the getKey stuff anymore.)

::: js/src/jit/CodeGenerator.cpp
@@ +3933,5 @@
> +        const JSJitInfo* jitInfo = target->jitInfo();
> +        if (jitInfo && jitInfo->type() == JSJitInfo::NoRetValNative)
> +            native = jitInfo->noRetValMethod;
> +    }
> +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, native));

Would it be possible to add a noRetVal argument to InvokeFunction in VMFunctions.cpp? That's the path we use when we don't know the target JSFunction* statically so supporting this optimization there would make us a bit more robust.

::: js/src/jit/IonBuilder.cpp
@@ +4918,5 @@
>      // If |Function.prototype.call| may be overridden, don't optimize callsite.
>      TemporaryTypeSet* calleeTypes = current->peek(calleeDepth)->resultTypeSet();
>      JSFunction* native = getSingleCallTarget(calleeTypes);
>      if (!native || !native->isNative() || native->native() != &fun_call) {
> +        CallInfo callInfo(alloc(), false, false);

It seems we could check BytecodeIsPopped here to optimize the splice.call(...) and splice.apply(...) cases too, right? Maybe file a follow-up bug?

Also it would be good to add /* constructing = */ and /* noRetVal = */ comments (or use enums) so this code becomes a bit more readable. Same a few times below.

::: js/src/jit/MIR.h
@@ +4080,5 @@
>  
>      // True if the call is for JSOP_NEW.
> +    bool construct_:1;
> +
> +    bool noRetVal_:1;

Nit: add a brief comment, something like:

// If true, the caller does not use the return value.

::: js/src/jsfriendapi.h
@@ +2396,3 @@
>      };
>  
> +    static unsigned offsetOfNative() {

Nit: rename Native to the name of the property we're accessing.

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8843657 [details] [diff] [review]
Part 2: Optimize Array.prototype.splice with JSOP_NORVCALL.

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

\o/

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add JSOP_CALL_IGNORES_RV for function call that ignores return value.

renamed:
  noRetVal => ignoresReturnValue
  JOSOP_NORVCALL => JSOP_CALL_IGNORES_RV
  JSJitInfo::NoRetValNative => JSJitInfo::IgnoresReturnValueNative
  offsetOfNative => offsetOfIgnoresReturnValueNative

about BaselineCallReturnAddr, added JitCompartment::hasBaselineCallReturnAddr because now it's shared between 2 ICCall_Fallback stubs (ignoresReturnValue_ == true/false, isConstructing == false) and we need to init it only once, from either of them.

now BytecodeIsPopped(pc) is passed to other CallInfos in IonBuilder.

and added ignoresReturnValue to InvokeFunction.

Comment 10

[Hannes Verschore [:h4writer]]

Great work. We should probably open a follow-up bug on this to iterate every MCall in IonMonkey and see if they have no uses and also test the fninfo for a "no return"-variant function. Might be we catch even more places where the output is not used.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8845910 [details] [diff] [review]
Part 1: Add JSOP_CALL_IGNORES_RV for function call that ignores return value.

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

Sorry for the delay! r=me with nits addressed.

::: js/src/jit/BaselineIC.cpp
@@ +2876,5 @@
>  
> +    // `ignoresReturnValue_ == false` case and `ignoresReturnValue_ == false`
> +    // case share baselineCallReturnAddr.
> +    if (cx->compartment()->jitCompartment()->hasBaselineCallReturnAddr(isConstructing_))
> +        return;

I think you can remove these 4 lines and everything should work fine. If not I'd like to understand why :)

::: js/src/jit/BaselineIC.h
@@ +755,5 @@
>              (1 << 18);  // constructing
>  
>        protected:
>          bool isConstructing_;
> +        bool ignoresReturnValue_;

I think you can remove this field. In DoCallFallback we have the pc/op so we can check that.

@@ +774,3 @@
>            : ICCallStubCompiler(cx, ICStub::Call_Fallback),
>              isConstructing_(isConstructing),
> +            ignoresReturnValue_(ignoresReturnValue),

Same here.

::: js/src/jit/JitCompartment.h
@@ +541,5 @@
>      void initBaselineCallReturnAddr(void* addr, bool constructing) {
>          MOZ_ASSERT(baselineCallReturnAddrs_[constructing] == nullptr);
>          baselineCallReturnAddrs_[constructing] = addr;
>      }
> +    bool hasBaselineCallReturnAddr(bool constructing) {

We can remove this too, see the other comment.

::: js/src/jit/VMFunctions.cpp
@@ +114,3 @@
>  
> +        return Call(cx, fval, thisv, args, rval);
> +    } else {

Nit: no else after return.

To get rid of the duplication, I think we should rename InvokeArgsIgnoresReturnValue to InvokeArgsMaybeIgnoresReturnValue and have it take an ignoresReturnValue argument? Then you can just pass it through and we don't have to duplicate the arguments initialization + Call :)

Comment 12

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #11)
> ::: js/src/jit/BaselineIC.cpp
> @@ +2876,5 @@
> >  
> > +    // `ignoresReturnValue_ == false` case and `ignoresReturnValue_ == false`
> > +    // case share baselineCallReturnAddr.
> > +    if (cx->compartment()->jitCompartment()->hasBaselineCallReturnAddr(isConstructing_))
> > +        return;
> 
> I think you can remove these 4 lines and everything should work fine. If not
> I'd like to understand why :)

yeah, I overlooked that it's no more used and there's no 2 variants.

Comment 13

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27e7cecf46420ac66d14134954c6d10e4a157911
Bug 1344477 - Part 1: Add JSOP_CALL_IGNORES_RV for function call that ignores return value. r=jandem

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebd6a21698472d184f4ff16fc5bcb9e83853ab68
Bug 1344477 - Part 2: Optimize Array.prototype.splice with JSOP_NORVCALL. r=jandem

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/27e7cecf4642
https://hg.mozilla.org/mozilla-central/rev/ebd6a2169847