1342070 - Don't create a dependent Promise if the return value of Promise.prototype.then or .catch isn't used

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, enhancement, P3)

Description

[Till Schneidereit [:till]]

then and catch are often called for their side effects, with the rval ignored. In those cases, we should just add the callbacks as reactions and not create a dependent promise at all.

Comment 1

[Tooru Fujisawa [:arai]]

will it make sense to check whether the return value is used or not in frontend (maybe after Parser?) and
emit different opcode for unused case?

I'm not sure how that information can be passed to native function with current native function's signature tho...

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: (concept) - Call dedicated native function when return value is not used.

here's very rough concept design that enables using different function for the case the return value is not used.

it uses JSJitInfo to store dedicated function for native function,
and calls it when the return value is not used.

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8841329 [details] [diff] [review]
(concept) - Call dedicated native function when return value is not used.

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

This is fantastic! Am I right in hoping that it's also possible to simplify handling of splice in ion now, ideally generalizing it to the point where it works for all functions having the right jitinfo?

Jandem, do you have any concerns about this approach?

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

Should this perhaps be packed with constructing_?

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8841329 [details] [diff] [review]
(concept) - Call dedicated native function when return value is not used.

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

This looks great to me too.

The next thing is to make the Baseline Call IC and IonBuilder call code swap the native with the no-rval version (if we have one), right?

Comment 5

[Tooru Fujisawa [:arai]]

Thank you for feedback :)

(In reply to Till Schneidereit [till] from comment #3)
> Am I right in hoping that it's also possible to simplify
> handling of splice in ion now, ideally generalizing it to the point where it
> works for all functions having the right jitinfo?

yeah, we could add array_splice_norval or something that calls array_splice_impl with returnValueIsUsed==false,
and just call it also in JIT, in more generic path.


> ::: js/public/CallArgs.h
> @@ +128,5 @@
> >    protected:
> >      Value* argv_;
> >      unsigned argc_;
> >      bool constructing_;
> > +    bool noRetVal_;
> 
> Should this perhaps be packed with constructing_?

yes


(In reply to Jan de Mooij [:jandem] from comment #4)
> Comment on attachment 8841329 [details] [diff] [review]
> (concept) - Call dedicated native function when return value is not used.
> The next thing is to make the Baseline Call IC and IonBuilder call code swap
> the native with the no-rval version (if we have one), right?

yes, I'll try that.

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8841329 [details] [diff] [review]
(concept) - Call dedicated native function when return value is not used.

the patch is moved to bug 1344477

Comment 7

[Tom S [:evilpie]]

Now that bug 1344477 landed can we use the patch to optimize Promises?

Comment 8

[Tooru Fujisawa [:arai]]

yes, providing the JSJitInfo like Array#splice will work.

Comment 9

[Tooru Fujisawa [:arai]]

forgot to note that the above obsolete patch doesn't implement the optimized case (just adds exactly same function).
so skipping the dependent Promise creation part is not yet implemented.

Comment 10

[Till Schneidereit [:till]]

Attachment: Part 1: Only create result Promises in Promise#then if it's used or the creation is otherwise observable

This depends on bug 1386534, largely because when implementing this I discovered how costly SpeciesConstructor is and added a fix for that issue separately.

The following micro-benchmark goes from ~90ms to ~70ms with this patch:

function bench() {
  var p;
  var t1 = Date.now();
  var p0 = Promise.resolve(1);
  function cb() {};
  for (var i = 100000; i--;)
    p0.then(cb); // Note the missing "p = " compared to the benchmark in bug 1386534.
  console.log(Date.now() - t1);
  return p;
}

bench();

Comment 11

[Till Schneidereit [:till]]

Attachment: Part 2: Only create result Promises in Promise#catch if it's used or the creation is otherwise observable

Same as part 1, but for catch. I needed to port catch to C++ to make this work, so the patch is slightly larger than otherwise expected.

Comment 12

[André Bargull [:anba]]

Comment on attachment 8892772 [details] [diff] [review]
Part 1: Only create result Promises in Promise#then if it's used or the creation is otherwise observable

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

Great!

::: js/src/builtin/Promise.cpp
@@ +2202,5 @@
>          if (!SpeciesConstructor(cx, promiseObj, JSProto_Promise, &ctorVal, originalSpecies))
>              return false;
>  
> +        if (createDependent == CreateDependentPromise::Always ||
> +            !IsNativeFunction(ctorVal, PromiseConstructor))

Because we're now skipping the complete promise object construction, we won't execute these lines [1]. Is that expected? (I'm just asking because I don't know anything about the debugger<->promise interactions.)


[1] http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/js/src/builtin/Promise.cpp#1269-1271

@@ +3192,5 @@
> +  { 0 }, /* unused */
> +  { 0 }, /* unused */
> +  JSJitInfo::IgnoresReturnValueNative,
> +  JSJitInfo::AliasEverything,
> +  JSVAL_TYPE_UNDEFINED,

Nit: Shouldn't this be indented by four spaces? I see that js::array_splice_info is also indented by just two spaces. Hmm, curious...

::: js/src/builtin/Promise.h
@@ +116,5 @@
>  GetWaitForAllPromise(JSContext* cx, const JS::AutoObjectVector& promises);
>  
> +enum class CreateDependentPromise {
> +    Always,
> +    SkipIfCtorUnobservable,

I don't feel too strongly about this, but maybe spell this out as "SkipIfConstructionUnobservable" or "SkipIfConstructorUnobservable".

Comment 13

[André Bargull [:anba]]

Comment on attachment 8892773 [details] [diff] [review]
Part 2: Only create result Promises in Promise#catch if it's used or the creation is otherwise observable

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

LGTM! 

I guess the extra cost of performing GetProperty() in native code compared to js-code is easily compensated by avoiding the then() call and the promise object construction? And Promise_catch can now be removed from Promise.js, right?

::: js/src/builtin/Promise.cpp
@@ +2088,5 @@
>  /**
>   * ES2016, 25.4.4.4, Promise.reject.
>   */
> +static bool
> +Promise_reject(JSContext* cx, unsigned argc, Value* vp)

Nit: Maybe rename to Promise_static_reject for consistency with resolve, all, and race?

@@ +3233,5 @@
>    JSVAL_TYPE_UNDEFINED,
>  };
>  
> +const JSJitInfo promise_catch_info = {
> +  { (JSJitGetterOp)Promise_catch_noRetVal },

Nit: Indent by four spaces?

@@ +3246,2 @@
>      JS_FNINFO("then", Promise_then, &promise_then_info, 2, 0),
> +    JS_FNINFO("catch", Promise_catch, &promise_catch_info, 1, 0),

Nit: Maybe keep "catch" before "then", so the property keys order stays the same?

::: js/src/builtin/Promise.h
@@ -213,5 @@
> -Promise_static_resolve(JSContext* cx, unsigned argc, Value* vp);
> -bool
> -Promise_reject(JSContext* cx, unsigned argc, Value* vp);
> -bool
> -Promise_then(JSContext* cx, unsigned argc, Value* vp);

Thanks for removing these from the header.

Comment 14

[Tooru Fujisawa [:arai]]

what's the status of this?

Comment 15

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #14)
> what's the status of this?

Sorry, I forgot about this :(

The implementation in SpiderMonkey is fine as-is, as far as it goes. The problem is that this causes tests to fail that verify that the dependent promise is properly reported as uncaught. The question is, should we simply change those tests and move on, or do something else? One thing we could do is still create the dependent promise if the global is being debugged. That'd lose some information, but not as much.

In any case, I'm afraid I don't have the time to work on this for the foreseeable future. Arai and André, would one of you be interested in taking over?

Comment 16

[Tooru Fujisawa [:arai]]

I'll take a look this weekend.

Comment 17

[Tooru Fujisawa [:arai]]

I'll be busy this weekend for bug 1193394.
I won't be able to look into this shortly.
if anyone is willing to, feel free to take over.

Comment 18

[Tooru Fujisawa [:arai]]

rebased:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=174f4d1a8cb452630f3f84bd84ce26a3909e64da

Comment 19

[Tooru Fujisawa [:arai]]

fixed compile error:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e80cd45609335364a9d081656ffd94a3e4fb652e

Comment 20

[Tooru Fujisawa [:arai]]

these tests are failing:

  * Performance Tools (Profiler/Timeline)
    * docshell/test/browser/browser_timelineMarkers-04.js
    * docshell/test/browser/browser_timelineMarkers-05.js
      => stack of the marker in profiler doesn't start from promise callback?

  * Inspector ?
    * devtools/client/inspector/markup/test/browser_markup_links_06.js
      => NS_ERROR_NOT_INITIALIZED is not thrown in somewhere

  * Console
    * devtools/client/webconsole/test/browser_webconsole_console_trace_async.js
      => stack of console.trace doesn't start from promise callback?

  * devtools/client/netmonitor/test/browser_net_cause.js
  * devtools/client/netmonitor/test/browser_net_frame.js
    => some item in stacktrace is undefined

so, basically it's something related to stack (except inspector case that I haven't looked into)
I'll check why such difference happens

Comment 21

[Tooru Fujisawa [:arai]]

So, except browser_markup_links_06.js, those tests depend on async stack, and async stack data is stored into Promise object.
If dependent Promise object is not created, that information is not available.
that results in the difference of the stack trace.

  * docshell/test/browser/browser_timelineMarkers-04.js
    https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/docshell/test/browser/browser_timelineMarkers-frame-04.js#34
  * docshell/test/browser/browser_timelineMarkers-05.js
    https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/docshell/test/browser/browser_timelineMarkers-frame-05.js#83
  * devtools/client/webconsole/test/browser_webconsole_console_trace_async.js
    https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/devtools/client/webconsole/test/browser_webconsole_console_trace_async.js#15
  * devtools/client/netmonitor/test/browser_net_cause.js
    https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/devtools/client/netmonitor/test/browser_net_cause.js#82
  * devtools/client/netmonitor/test/browser_net_frame.js
    https://searchfox.org/mozilla-central/rev/877c99c523a054419ec964d4dfb3f0cadac9d497/devtools/client/netmonitor/test/browser_net_frame.js#151

Comment 22

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #21)
> [T]hose tests depend on async stack,
> and async stack data is stored into Promise object.
> If dependent Promise object is not created, that information is not
> available.

Ah, right. So perhaps we could only create dependent promises if async stacks are enabled? It seems like that'd fix theses tests, right?

Comment 23

[Tooru Fujisawa [:arai]]

(In reply to Till Schneidereit [:till] from comment #22)
> (In reply to Tooru Fujisawa [:arai] from comment #21)
> > [T]hose tests depend on async stack,
> > and async stack data is stored into Promise object.
> > If dependent Promise object is not created, that information is not
> > available.
> 
> Ah, right. So perhaps we could only create dependent promises if async
> stacks are enabled? It seems like that'd fix theses tests, right?

yeah, that's possible, but in that case this optimization isn't enabled on Nightly.

Comment 24

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #23)
> yeah, that's possible, but in that case this optimization isn't enabled on
> Nightly.

Right, we should really fix bug 1280819 :/

I think that's the right thing to do here: fix this in the straight-forward way ignoring failures to optimize because of async stacks being enabled, and have those concerns be fixed by fixing the underlying issue.

Comment 25

[Tooru Fujisawa [:arai]]

The another workaround here will be, just to use the value returned by then/catch, in those tests.

On browser_timelineMarkers-04.js, the function caught in the timeline is the following anonymous function, and the asyncParent is the Promise returned by Promise#then.

https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/docshell/test/browser/timelineMarkers-04.html#44-46
>     function do_promise_script() {
>       new Promise(...
> ...
>       ...).then(function (val) {
>         console.timeEnd("Bob");
>       });

just assigning the return value to local variable solves the failure.
how do you think about this approach?

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Use the return value of Promise#then in testcases to avoid unexpected optimization.

fwiw, here's the workaround patch that assigns the return value of Promise#then,
to disable the optimization and keep the async stack.

Comment 27

[Till Schneidereit [:till]]

(In reply to Tooru Fujisawa [:arai] from comment #25)
> just assigning the return value to local variable solves the failure.
> how do you think about this approach?

Yeah, that seems like a perfectly fine solution to me.

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Use the return value of Promise#then in testcases to avoid unexpected optimization.

To workaround the issue that, when the Promise object creation is optimized out, the stack observed in devtools profiler etc lacks the promise reaction,
I've added the following fix:

devtools/client/netmonitor/test/html_cause-test-page.html
devtools/client/netmonitor/test/html_frame-subdocument.html
devtools/client/netmonitor/test/html_frame-test-page.html
devtools/client/webconsole/test/test-console-trace-async.html
docshell/test/browser/timelineMarkers-04.html
  assign the Promise returned by Promise#then to local variable,
  to suppress the optimization that is applied when the returned Promise is not used.

docshell/test/browser/browser_timelineMarkers-frame-05.js
  same as above
  also updated the functionDisplayName, since the assigned variable name is reflected there

devtools/client/inspector/markup/test/browser_markup_links_06.js
  NS_ERROR_NOT_INITIALIZED is no more thrown, so just removed it.
  I'm not sure if it's right fix tho

devtools/client/netmonitor/test/browser_net_cause.js
devtools/client/netmonitor/test/browser_net_frame.js
devtools/client/webconsole/test/browser_webconsole_console_trace_async.js
  update line numbers, that is changed by html changes above

Comment 29

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Only create result Promises in Promise#then if it's used or the creation is otherwise observable. r=anba

and here's rebased patches

Comment 30

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Only create result Promises in Promise#catch if it's used or the creation is otherwise observable. r=anba

Comment 31

[Alexandre Poirot [:ochameau]]

Comment on attachment 8964775 [details] [diff] [review]
Part 0: Use the return value of Promise#then in testcases to avoid unexpected optimization.

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

This patch fixes the tests, but developers will start seeing empty or incomplete stacks and it doesn't seem reasonable to explain everyone (firefox and web developers) that they have to create an intermediate variable to get valid stacks on Firefox.
Could we keep full featured stacks when devtools are enabled? Or when javascript.options.asyncstack is true?
I can help on how to know if devtools are on or off.

At the same time, if this patch brings improvements even if asyncstack pref is false,
it looks like a very interesting patch. We may followup to bring back full stacks back if that helps.

Comment 32

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)

(In reply to Alexandre Poirot [:ochameau] from comment #31)
> Could we keep full featured stacks when devtools are enabled? Or when
> javascript.options.asyncstack is true?
> I can help on how to know if devtools are on or off.

Thanks, these tests no more fail when applying the optimization only if the current compartment is not debuggee:
 * devtools/client/webconsole/test/browser_webconsole_console_trace_async.js
 * devtools/client/netmonitor/test/browser_net_cause.js
 * devtools/client/netmonitor/test/browser_net_frame.js
(I think we can enable this optimization if javascript.options.asyncstack==false, even if it's debuggee)

This one still needs the same fix, that is just to remove expected error:
 * devtools/client/inspector/markup/test/browser_markup_links_06.js

These tests are not fixed, because devtools is not opened:
 * docshell/test/browser/browser_timelineMarkers-04.js
 * docshell/test/browser/browser_timelineMarkers-05.js

I'll investigate more about the last 2.


> At the same time, if this patch brings improvements even if asyncstack pref
> is false,
> it looks like a very interesting patch. We may followup to bring back full
> stacks back if that helps.

This patch removes unnecessary object allocations, regardless of asyncstack pref value.

Comment 33

[Tooru Fujisawa [:arai]]

Okay, so we can apply the optimization if the following conditions are met:
  * devtools is not opened (the current compartment is not debuggee)
  * GeckoProfiler is not enabled
  * Profile Timeline Recording is not enabled

Actually, the stack will also be observable by Error#stack, but that's non-standard and also the difference is already there between release/beta and nightly, so I think we can ignore it.

here's try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e57e74fd4d0e2e3421ed8afbddcd57584507698

Comment 34

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Add API to tell Profile Timeline Recording state to JS engine.

The stack information is observable in Profile Timeline Recording (is it correct name?),
and I want to disable the optimization if both it and async stack are enabled.

The tests failing because of this issue are:
 * docshell/test/browser/browser_timelineMarkers-04.js
 * docshell/test/browser/browser_timelineMarkers-05.js

and they set `docShell.recordProfileTimelineMarkers`,
so what I want to actually check is that value, but looks like it's not associated with JS context or runtime.
and also looks like there can be multiple instances that it's set to true at the same time, according to its implemenation.

So I added the following APIs:

  * SetProfileTimelineRecordingEnabled
    tells JS engine about the state of Profile Timeline Recording
    this is global state
    this is called in TimelineConsumers::{AddConsumer,RemoveConsumer},
    when the number of active consumers becomes 0->1 or 1->0

  * IsProfileTimelineRecordingEnabled
    returns whether Profile Timeline Recording is enabled
    this function will be used in Promise impl to stop optimization if necessary

Comment 35

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Disable optimization if devtools or profiler is used.

Now the optimization can be enabled only if the returned Promise is not used even implicitly by debugger or profiler.
Added IsPromiseThenOrCatchRetValImplicitlyUsed function that returns whether return value is implicitly used by one of them,
and called it in Promise_then_noRetVal and Promise_catch_noRetVal.

Comment 36

[Tooru Fujisawa [:arai]]

Attachment: Part 4: Remove unnecessary PromiseTestUtils.expectUncaughtRejection call.

The remaining fix is for devtools/client/inspector/markup/test/browser_markup_links_06.js
this patch just removes PromiseTestUtils.expectUncaughtRejection call,
since NS_ERROR_NOT_INITIALIZED is no more thrown.

Then, this test fails on test-verify, even without the fix.
So I'd like to leave it to other bug.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e57e74fd4d0e2e3421ed8afbddcd57584507698&group_state=expanded&selectedJob=172028484

Comment 37

[Alexandre Poirot [:ochameau]]

Comment on attachment 8965535 [details] [diff] [review]
Part 3: Disable optimization if devtools or profiler is used.

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

::: js/src/builtin/Promise.cpp
@@ +3063,5 @@
> +    // and profilers.  We shouldn't apply the optimization not to allocate the
> +    // returned Promise object if the it's implicitly used by them.
> +    //
> +    // FIXME: Once bug 1280819 gets fixed, we can use ShouldCaptureDebugInfo.
> +    if (!cx->options().asyncStack())

Will asyncStack return false when javascript.options.asyncstack is false?
It would be great if devtools only force the slow async stack code all around only if the pref is true.

Comment 38

[Alexandre Poirot [:ochameau]]

Comment on attachment 8965536 [details] [diff] [review]
Part 4: Remove unnecessary PromiseTestUtils.expectUncaughtRejection call.

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

Do you still need this?
It is surprising to see your patch still have an impact on DevTools.
Anyway it is good to get rid of such random exception.

> Then, this test fails on test-verify, even without the fix.
> So I'd like to leave it to other bug.

Sounds good.

Comment 39

[Tom Tromey :tromey]

Comment on attachment 8965534 [details] [diff] [review]
Part 0: Add API to tell Profile Timeline Recording state to JS engine.

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

This looks good to me, though note that I think the timeline is probably going to go away.
So, maybe regressing that test is actually fine.
I will NI the appropriate person to find out.

::: js/src/jsapi.cpp
@@ +1352,5 @@
>      thisObject.set(&thisv.toObject());
>      return true;
>  }
>  
> +bool gProfileTimelineRecordingEnabled = false;

I think this should be "static".

Comment 40

[Tom Tromey :tromey]

Greg - could you comment on comment #39?

Comment 41

[Greg Tatum [:gregtatum]]

It probably will be removed eventually, but we're not in any hurry to do so right now, so I'd prefer to keep tests working since we're not touching the DevTools performance code much right now. I didn't look into this issue too much, but if it's not a big deal to fix, that would be my preference. Let me know if you want me to look into it more.

Comment 42

[Till Schneidereit [:till]]

Comment on attachment 8965534 [details] [diff] [review]
Part 0: Add API to tell Profile Timeline Recording state to JS engine.

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

r=me with ttromey's comment addressed.

Comment 43

[Till Schneidereit [:till]]

Comment on attachment 8965535 [details] [diff] [review]
Part 3: Disable optimization if devtools or profiler is used.

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

r=me

@ttromey: yes, cx->options().asyncStack() reflects that pref.

Comment 44

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ebe4719c2414dc32568d67a2889c48adb686031
Bug 1342070 - Part 0: Add API to tell Profile Timeline Recording state to JS engine. r=till,tromey

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2d33789539668305c1e8ac2d65a05a590c8a548
Bug 1342070 - Part 1: Only create result Promises in Promise#then if it's used or the creation is otherwise observable. r=anba

https://hg.mozilla.org/integration/mozilla-inbound/rev/f054209125461575f57ce0f1e6b962653c523572
Bug 1342070 - Part 2: Only create result Promises in Promise#catch if it's used or the creation is otherwise observable. r=anba

https://hg.mozilla.org/integration/mozilla-inbound/rev/e66e4b3ee60f04411e0df5d446794d4166c3ba80
Bug 1342070 - Part 3: Disable optimization if devtools or profiler is used. r=till

https://hg.mozilla.org/integration/mozilla-inbound/rev/7674892ac2302b03c75cae1174bff06bffe280ea
Bug 1342070 - Part 4: Remove unnecessary PromiseTestUtils.expectUncaughtRejection call. r=ochameau

Comment 45

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8ebe4719c241
https://hg.mozilla.org/mozilla-central/rev/c2d337895396
https://hg.mozilla.org/mozilla-central/rev/f05420912546
https://hg.mozilla.org/mozilla-central/rev/e66e4b3ee60f
https://hg.mozilla.org/mozilla-central/rev/7674892ac230