1405999 - Some Promise optimizations

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

A bunch of patches that improve the micro-benchmark below from ~328 ms to ~254 ms with --no-async-stacks.

function f() {
    var g = (r) => void 0;
    var count = 1000000;
    var start = Date.now();
    for (var i = 0; i < count; ++i) {
        new Promise(g);
    }
    var stop = Date.now();
    print(stop - start);
}
f();

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Optimize CreatePromiseObjectInternal

* Eliminates some roots.
* Uses initFixedSlot instead of setFixedSlot.
* Adds MOZ_ALWAYS_INLINE to CreatePromiseObjectInternal

All of these make a small but measurable difference.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Optimize CreateResolvingFunctions

More of the same, but for CreateResolvingFunctions.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Some GC changes

This moves the AutoGCRooter ctors to the header file so we can inline them. I also had to move the code after the RootingContext definition - the diff here is pretty bad unfortunately but except for the ctor changes it's just moving code.

This also adds MOZ_ALWAYS_INLINE to some post barrier methods that Clang was not inlining. We really want to inline these so the compiler can get rid of barriers when storing Int32Value or something.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Optimize NewNativeFunction et al

* Moves NewNativeFunction and NewNativeConstructor to the header file, since they're trivial and this avoids an unnecessary extra call.

* The NewFunctionProtoHandling argument was always NewFunctionClassProto, so I removed the enum + the argument.

* Removes unnecessary roots.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8915536 [details] [diff] [review]
Part 3 - Some GC changes

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

Looks good to me.  Thanks for fixing the inlining.

Comment 6

[André Bargull [:anba]]

Comment on attachment 8915538 [details] [diff] [review]
Part 4 - Optimize NewNativeFunction et al

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

LGTM!

::: js/src/jsfun.h
@@ +686,5 @@
>  
>  extern bool
>  AsyncGeneratorConstructor(JSContext* cx, unsigned argc, Value* vp);
>  
> +// If  enclosingEnv is null, the function will have a null environment()

Nit: double space between "If" and "enclosingEnv"

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 8915534 [details] [diff] [review]
Part 1 - Optimize CreatePromiseObjectInternal

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

It's interesting how much of a difference rooting does make in cases like this.

::: js/src/builtin/Promise.cpp
@@ +1487,5 @@
>  
>      // Step 7.
>      // Implicit, the handled flag is unset by default.
>  
> +    if (MOZ_LIKELY(!ShouldCaptureDebugInfo(cx)))

Can you add an assert that !informDebugger here?

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8915535 [details] [diff] [review]
Part 2 - Optimize CreateResolvingFunctions

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

r=me

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Till Schneidereit [:till] from comment #7)
> > +    if (MOZ_LIKELY(!ShouldCaptureDebugInfo(cx)))
> 
> Can you add an assert that !informDebugger here?

I'm not sure if that holds. What makes this change okay is: (1) ShouldCaptureDebugInfo always returns |true| if cx->compartment()->isDebuggee() and (2) Debugger::onNewPromise is a no-op if cx->compartment()->isDebuggee() is false. IOW, if ShouldCaptureDebugInfo returns false, we know Debugger::onNewPromise is a no-op.

Comment 10

[Till Schneidereit [:till]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> I'm not sure if that holds. What makes this change okay is: (1)
> ShouldCaptureDebugInfo always returns |true| if
> cx->compartment()->isDebuggee() and (2) Debugger::onNewPromise is a no-op if
> cx->compartment()->isDebuggee() is false. IOW, if ShouldCaptureDebugInfo
> returns false, we know Debugger::onNewPromise is a no-op.

Oh, I see. I hadn't considered that possibility, thanks for explaining.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/373a038aafbd
part 1 - Optimize CreatePromiseObjectInternal. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/29f89eb9deab
part 2 - Optimize CreateResolvingFunctions. r=till
https://hg.mozilla.org/integration/mozilla-inbound/rev/93a3c28a68a7
part 3 - Inline AutoGCRooter constructors and add MOZ_ALWAYS_INLINE to some functions. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/45efb288d8c5
part 4 - Optimize and clean up NewNativeFunction and related functions. r=anba

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/373a038aafbd
https://hg.mozilla.org/mozilla-central/rev/29f89eb9deab
https://hg.mozilla.org/mozilla-central/rev/93a3c28a68a7
https://hg.mozilla.org/mozilla-central/rev/45efb288d8c5