Some Promise optimizations

RESOLVED FIXED in Firefox 58

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 2 bugs)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

2 years ago
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();
Assignee

Comment 1

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

All of these make a small but measurable difference.
Attachment #8915534 - Flags: review?(till)
Assignee

Comment 2

2 years ago
More of the same, but for CreateResolvingFunctions.
Attachment #8915535 - Flags: review?(till)
Assignee

Comment 3

2 years ago
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.
Attachment #8915536 - Flags: review?(jcoppeard)
Assignee

Comment 4

2 years ago
* 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.
Attachment #8915538 - Flags: review?(andrebargull)
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.
Attachment #8915536 - Flags: review?(jcoppeard) → review+
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"
Attachment #8915538 - Flags: review?(andrebargull) → review+
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?
Attachment #8915534 - Flags: review?(till) → review+
Comment on attachment 8915535 [details] [diff] [review]
Part 2 - Optimize CreateResolvingFunctions

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

r=me
Attachment #8915535 - Flags: review?(till) → review+
Assignee

Comment 9

2 years ago
(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.
(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

2 years ago
Pushed by jandemooij@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.