[Idea] Avoid redundant HostPromiseRejectionTrackerEvents

[stefanpenner]

context: https://twitter.com/bmeurer/status/960440074574934016

cc @bmeurer / @domenic / @littledan

Motivation

To mitigate a performance concern @bmeurer raised, related to what I believe redundant HostPromiseRejectionTracker events.

Lets discuss!

Changes

This does change behavior slightly:

• Rejections which become handled before the rejections PromiseReactionJob runs will no longer be visible to the HostPromiseRejectionTracker.
• The HostPromiseRejectTracker no longer being informed synchronously, rather being informed during the associated PromiseReactionJob.

On a positive note, there should be nearly no overhead in this approach.

Example:

Promise.reject().catch();

Would previously emit paired reject and handle events, and now emits neither.

Some in-the-wild code: https://github.com/nodejs/node/blob/d62566e1b1bb4734635d51d53c16f6efa3a7f5b5/lib/internal/process/promises.js#L26-L42

I believe would continue to function as implemented, the difference being maybeUnhandledPromises would not contain promises which have become handled by the time their PromiseReactionJob has been processed.

[ljharb]

This seems like a great change to me that reduces that number of times a host would have to clean up after a false unhandled rejection, and minimizes error noise.

[bmeurer]

I really love this simplification, but I'm not sure about the implications for embedders. For example Node collects the stack trace in the "reject" handler and reuses that later. Not sure how that affects the user experience there.

[littledan]

I share Benedikt's concern about losing stack traces. Not sure whether anyone has implemented this yet, but it may be possible to collect post-mortem dumps from here too.

I wonder whether it might be possible to use catch prediction to avoid collecting expensive traces, though this means leaving (at least part of) devtools on all the time. Catch prediction would be less trigger-happy for async functions, but still get tripped up by Promise.reject().catch(() => {}).

[littledan]

I think this PR wouldn't make any observable changes for the unhandledrejection event (which is how the web platform uses this hook), because notify about rejected promises is executed only after draining the microtask queue, but @domenic would be able to verify this.

[domenic]

I'd rather implementations make unobservable optimizations like this behind the scenes, instead of requiring spec changes that cascade to host specs. In other words, keep the ES spec as low level as it currently is, instead of trying to do optimizations in spec land.

[stefanpenner]

I'd rather implementations make unobservable optimizations like this behind the scenes

Does the committee consider dropping redundant paired reject/handle signals to HostPromiseRejectionTracker unobservable?

If so, some guidance for implementors may be required (potentially just some consensus in this thread). As the above discussion suggests, implementers may not have the context, or do not realize they have the freedom make the above change.

I'd rather implementations make unobservable optimizations like this behind the scenes, instead of requiring spec changes that cascade to host specs. In other words, keep the ES spec as low level as it currently is, instead of trying to do optimizations in spec land.

@domenic what you say isn't unreasonable, but help me understand more exactly, do you mean:

• spec is fine, it allows for this already, or
• spec isn't fine, but the cost to change is to high, so implementors can make needed adjustments, or
• spec is fine, implementors who diverge beware, or
• ... or I missed the point ...

[domenic]

I mean the first.

In general, if you can't write a test whose behavior changes with a spec change, then you're making unobservable changes to the spec. And what I'm saying is I'd prefer not to do that.

[stefanpenner]

I mean the first.

If the spec allows for it, I can agree with it. Lets hear from implementors.

[stefanpenner]

@domenic I suspect the concerns are related to observable differences from the perspective of embedders. Is it fair to say, the spec both doesn't and shouldn't care about differences for embedders? And due to optimizations as suggested above, those differences are just a reality.

[ljharb]

Do we know which implementations would be willing to implement this optimization, in the absence of this PR?

[domenic]

I think that's going a bit far. What the spec should not care about is how it's observable consequences get implemented. How those changes manifest for the host integration points is subtle and spans multiple specs.

As such, I think that any changes to host interfaces should be done in careful collaboration with host specs that are using those interfaces. Here, as far as I can tell, the desired end result is that everyone coordinates on spec changes to produce... No difference in behavior.

In that case I'd rather not change things.

[bterlson]

It is not fair to say that the spec doesn't/shouldn't care about host interoperability. We definitely care. However, this seems like the wrong layer to drive this change.

What you propose is already fine per the existing spec text. I would rather consider this later with actual interop data and an idea of how changes would positively impact that then to sign up for speccing the actual semantics of promise rejection tracking beyond that such a capability exists.

[littledan]

This hook is used by multiple ECMAScript embedding environments--not just the Web Platform but also Node.js. For ES implementations with an API which matches the spec's hooks (like V8 with respect to this hook), this would be an observable change, with particular implications for non-Web embedders, even if the Web uses the hook in a way which would not show a change. I don't really see the change as being more low-level or more high-level, it's just different.

It's true that ES implementers could expose a different hook to embedders and still ship the ES+HTML semantics anyway, but there's something nice about being able to have some documentation that says, "this API stands in for this heading in the spec text"--you know what you're getting.

[domenic]

sign up for speccing the actual semantics of promise rejection tracking beyond that such a capability exists.

I really like this framing in particular. The existing spec is very minimal and is basically saying "hosts have the ability to track promise rejections". What's being proposed here is getting much further into the details of how specific known hosts (OP motivated by Node.js, and is hopefully compatible with the web too) choose to manifest their rejection tracking, e.g. by batching the notifications and canceling each other out. Those considerations seem best left to the hosts, and to the JS engines that support them.

[littledan]

@domenic Do you think this PR will change semantics for the Web embedding?

[domenic]

What do you mean by "semantics"?

[littledan]

@domenic Would this PR "break" or change how/when onunhandledrejection is sent? My reading: no. But you're the expert here.

[domenic]

My understanding is that, by coordinating changes across all host environments, this PR would cause no web-observable differences. I haven't spent the time looking into what changes on both sides would be required though to preserve that parity---and I'd rather not do so, as just changing a bunch of things for no observable effect is not, IMO, a good use of my time.

[bmeurer]

@bterlson The problem with HostPromiseRejectionTracker from the point of view of a JSVM is that it's almost completely unspecified what can happen in there. The only requirement is that it must not throw an exception. So for a compliant implementation that means it has to assume that anything can happen while HostPromiseRejectionTracker is running, arbitrary JavaScript can be executed, the state of the system can change completely.

So unless the JSVM imposes additional restrictions or decides not to implement this part of the spec, it cannot really optimize away/across this host call.

I'm not sure yet what is better, at least from V8's perspective (and speaking specifically about the Node use case), but it seems rather unfortunate that you get penalized for attaching a handler to a rejected promise. My personal opinion here is that this shouldn't have been in the spec at all, as it's kinda inconsistent with the other debugging features, for example async stack traces or general debug stepping is all implemented under the hood in the engines/hosts without the spec mentioning it explicitly. So why do we have this particular host interaction specified? I guess there's a reason for that?

cc @hashseed

[domenic]

It seems like there's some confusion about the purpose of this part of the spec. It's not meant to be implemented verbatim, because (like other debugging features you note), it's not observable.

What is observable is the unhandledrejection and rejectionhandled events specified in HTML. And those need to get information from the ES spec in order to be well-specified. That's what this hook is for.

So as long as the observable consequences (specified in HTML) are the same, the JSVM is fulfilling its duties to its host. The JSVM doesn't need to follow the specification here verbatim. Just like it doesn't need to follow verbatim the specification for Map/Set's List of entry-records.

Stated another way, how would you specify HTML's unhandledrejection/rejectionhandled events without this hook? It's part of the interoperable web platform, so it needs to be specified. And it needs this hook to be well-specified. That's why this hook exists: to help specify the observable consequences of HTML's events. Not as some kind of guide to what V8's embedder API should look like.

Edit: where does Node fit in? Well, Node doesn't really need this hook, because it doesn't have a specification for interoperable multi-implementation behavior, that needs to build on top of the ES spec. It can just talk directly to V8, using whatever API it wants. It already does that for lots of things, e.g. debugger, REPL, etc., and we don't spec those. How Node wants to communicate promise rejections to V8 is totally up to Node/V8, and not really in the domain of the spec, just like the debugger interactions are not. The hook being in the spec is only necessary for hosts whose behavior is governed by specifications, instead of by directly working with their underlying JS VM.

[bterlson]

@bmeurer you can find additional context in the original PR here: #76. The main motivation was to improve layering between various specifications.

I would also note that, "it has to assume that anything can happen while HostPromiseRejectionTracker is running, arbitrary JavaScript can be executed, the state of the system can change completely." is an implementation-specific concern of promise-rejection tracking that allows such script to run.

[bmeurer]

@bterlson, @domenic Thanks, that makes total sense now. Maybe we just need to limit what is allowed to happen during HostPromiseRejectionTracker. But as said earlier, that blocks on the more serious problem of how to do post-mortem debugging properly with promises.

[littledan]

@domenic

My understanding is that, by coordinating changes across all host environments, this PR would cause no web-observable differences. I haven't spent the time looking into what changes on both sides would be required though to preserve that parity---and I'd rather not do so, as just changing a bunch of things for no observable effect is not, IMO, a good use of my time.

My reading was that nothing would have to change on the other side. I understand if you don't have time to look into this right now, though.

@bmeurer

But as said earlier, that blocks on the more serious problem of how to do post-mortem debugging properly with promises.

Let's keep discussing that issue at nodejs/post-mortem#45

[bterlson]

@stefanpenner what do you think about closing this for now based on above discussion?

[stefanpenner]

@stefanpenner what do you think about closing this for now based on above discussion?

Works for me, glad this conversation happened!