lib,src: replace all C++ promises with JS promises #20830

BridgeAR · 2018-05-18T23:36:27Z

C++ promises can not be properly optimized by V8. They also behave
a tiny bit different than "regular" promises (they can not be monkey patched).

This was requested by @nodejs/v8.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

BridgeAR · 2018-05-19T00:10:28Z

CI https://ci.nodejs.org/job/node-test-pull-request/14968/

TimothyGu · 2018-05-19T00:43:38Z

This was requested by @nodejs/v8.

Can you link where as a "Refs" in the commit message?

BridgeAR · 2018-05-19T00:50:33Z

@TimothyGu we discussed this in person (even though we have meeting notes) two weeks ago.

TimothyGu · 2018-05-19T00:55:21Z

They also behave a tiny bit different than "regular" promises (they can not be monkey patched).

Isn't this a desirable property?

BridgeAR · 2018-05-19T01:08:46Z

@TimothyGu not really. Monkey patching is for example still important for APMs and there are probably a couple use cases where people rely on being able to monkey patch promises. Not being able to monkey patch those goes AFAIK against the spec.

TimothyGu · 2018-05-19T01:13:43Z

Monkey patching is for example still important for APMs

Async Hooks are also important to APMs, and monkey patching Promise would not allow PROMISE hooks from being called as expected. (By saying this I'm not saying not monkeypatching is better for APMs – only APM developers can say for sure. I am saying this should not be listed as one of the pros for allowing monkeypatching.)

Not being able to monkey patch those goes AFAIK against the spec.

There is no spec for util.promisify, except the documentation and our previous behavior, the latter of which this PR is now breaking.

devsnek · 2018-05-19T01:36:15Z

a happy medium that can let changing these to be monkey-patchable in a separate pr could be using SafePromise from internal/safe_globals

TimothyGu · 2018-05-19T01:45:26Z

@devsnek We don't want to expose any Safe* objects to userland, as it would no longer be safe. Simply caching the Promise constructor at startup should suffice.

benjamingr · 2018-05-19T07:16:05Z

First - do we have benchmarks for this?

Second - monkey patching won’t work because of async functions anyway - the future for diagnostics is better hooks

Otherwise LGTM

benjamingr · 2018-05-19T07:17:02Z

lib/child_process.js

-      } else {
-        promiseResolve(promise, { stdout, stderr });
-      }
+    return new Promise((resolve, reject) => {


It was written that way to avoid allocation of a closure and be speed-competitive with bluebird.

I’d like to make sure that the speed remains as fast

Yes, we should measure the performance here.

I was suggesting to start with JS promises instead, since we spend some time recently to even inline the promise constructor into TurboFan optimized code. If that turns out to be too slow, then the other alternative is to use the createPromise and friends from v8-extras, which is also known to TurboFan, and significantly faster than going through C++.

+1 for createPromise from v8-extras.

lib/internal/util.js

-      promiseReject(promise, err);
-    }
-    return promise;
+    return new Promise((resolve, reject) => {


bmeurer

LGTM if performance is okay.

benjamingr

I'm only blocking to make sure no one merges this by accident before benchmarks.

apapirovski

This will be somewhere around 10-30% slower than the current version. I tested this fairly recently in relation to promisify. I'll leave it up to others to decide whether that's acceptable.

I chose to discard my own PR due to how much slower it was but perhaps others have a different opinion.

benjamingr · 2018-05-19T11:00:30Z

@apapirovski

This will be somewhere around 10-30% slower than the current version. I tested this fairly recently in relation to promisify. I'll leave it up to others to decide whether that's acceptable.

A 30% slower promisify would beat half the point of having promisify itself in core. While I personally find it a useful utility anyway the speed benefit was a big part of the reason people were convinced to add it.

That is, if this PR is significantly slower then I'm very -1 on it. However if the V8 team can make an in-javascript promisify as fast as the current version then I'm +1 on it - and in any case a v8-extras solution should be considered.

jasnell · 2018-05-19T16:22:40Z

@BridgeAR ... would prefer the changes to different modules to at least be in separate commits.

BridgeAR · 2018-05-19T16:36:59Z

@jasnell if this will land at all I would like to keep it in a single commit. I do not see a compelling reason for it to be split into smaller chunks.

@apapirovski do you still have the benchmark code you used? Any performance impact is heavily influenced by the task at hand. So benchmarking a promisified setTimeout should show the biggest impact if it has influence on the performance.

A general thing: I feel we block to many things for reasons that will have little to no effect on real world applications.

jasnell · 2018-05-19T16:53:57Z

The compelling reason is to isolate distinct changes in case they need to be reverted or backported separately for any reason. It also makes the changes easier to review module by module.

addaleax · 2018-05-19T16:56:35Z

@jasnell I think this is fine to land as a single commit – it’s not particularly huge or so, and I don’t think we want to backport any of this anyway because of the perf impact (adding dont-land-on labels).

TimothyGu · 2018-06-26T16:22:32Z

lib/internal/http2/core.js

@@ -2731,11 +2730,9 @@ function connect(authority, options, listener) {
 // Support util.promisify
 Object.defineProperty(connect, promisify.custom, {
  value: (authority, options) => {
-    const promise = createPromise();
-    const server = connect(authority,


Actually, it seems we might be able to remove the custom promisifier entirely: what the getter is doing right now should be the exact things done by util.promisify.

A test fails if that part is removed.

C++ promises can not be properly optimized by V8. They also behave a tiny bit different than "regular" promises.

BridgeAR · 2018-07-14T01:53:07Z

CI https://ci.nodejs.org/job/node-test-pull-request/15870/

TimothyGu · 2018-07-15T17:17:51Z

@BridgeAR Can you file a follow-up issue on #20830 (comment) when this PR gets landed?

BridgeAR · 2018-07-16T14:46:18Z

Landed in bd1f355 🎉

C++ promises can not be properly optimized by V8. They also behave a tiny bit different than "regular" promises. PR-URL: nodejs#20830 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benedikt Meurer <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

This is just a small refactoring. Refs: nodejs#20830 (comment)

BridgeAR · 2018-07-16T15:45:31Z

@TimothyGu I personally can not see how that custom promisify should be removed. It is a wrapper for an event and the first argument is always going to be the server. Therefore the http2 case relies on the return value of the function and not on the callback value as that would be registered as an error.
However, I saw that the function could be minified further and I did that: #21835

If there is something else you had in mind, please let me know!

targos · 2018-07-16T19:26:00Z

Should this be backported to v10.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

C++ promises can not be properly optimized by V8. They also behave a tiny bit different than "regular" promises. PR-URL: nodejs#20830 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benedikt Meurer <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

C++ promises can not be properly optimized by V8. They also behave a tiny bit different than "regular" promises. PR-URL: #20830 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Benedikt Meurer <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Backport-PR-URL: #21879 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

BridgeAR requested review from bmeurer, benjamingr and hashseed May 18, 2018 23:36

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 18, 2018

addaleax approved these changes May 18, 2018

View reviewed changes

devsnek approved these changes May 18, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 19, 2018

TimothyGu approved these changes May 19, 2018

View reviewed changes

benjamingr reviewed May 19, 2018

View reviewed changes

lib/internal/util.js Outdated

promiseReject(promise, err);

}

return promise;

return new Promise((resolve, reject) => {

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

bmeurer approved these changes May 19, 2018

View reviewed changes

benjamingr requested changes May 19, 2018

View reviewed changes

apapirovski reviewed May 19, 2018

View reviewed changes

addaleax added dont-land-on-v6.x labels May 19, 2018

TimothyGu reviewed Jun 26, 2018

View reviewed changes

lib,src: replace all C++ promises with JS promises

fb1d667

C++ promises can not be properly optimized by V8. They also behave a tiny bit different than "regular" promises.

BridgeAR force-pushed the replace-c-promises branch from 5627934 to fb1d667 Compare July 13, 2018 18:38

BridgeAR closed this Jul 16, 2018

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jul 16, 2018

http2: simplify custom promisify

ce447c3

This is just a small refactoring. Refs: nodejs#20830 (comment)

BridgeAR mentioned this pull request Jul 16, 2018

http2: simplify custom promisify #21835

Closed

4 tasks

targos added backport-requested-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 16, 2018

targos mentioned this pull request Jul 19, 2018

test: make crashOnUnhandleRejection opt-out #21849

Closed

4 tasks

targos mentioned this pull request Jul 19, 2018

[v10.x] lib,src: replace all C++ promises with JS promises #21879

Closed

targos added backported-to-v10.x and removed backport-requested-v10.x labels Jul 24, 2018

targos mentioned this pull request Jul 31, 2018

v10.8.0 proposal #22040

Merged

benjamingr mentioned this pull request Oct 11, 2018

Avoid the generic promisify. v8/promise-performance-tests#2

Merged

BridgeAR deleted the replace-c-promises branch January 20, 2020 11:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib,src: replace all C++ promises with JS promises #20830

lib,src: replace all C++ promises with JS promises #20830

BridgeAR commented May 18, 2018

BridgeAR commented May 19, 2018

TimothyGu commented May 19, 2018

BridgeAR commented May 19, 2018

TimothyGu commented May 19, 2018

BridgeAR commented May 19, 2018

TimothyGu commented May 19, 2018

devsnek commented May 19, 2018 •

edited

Loading

TimothyGu commented May 19, 2018

benjamingr commented May 19, 2018

benjamingr May 19, 2018

bmeurer May 19, 2018

benjamingr May 19, 2018

This comment was marked as outdated.

This comment was marked as outdated.

bmeurer left a comment

benjamingr left a comment

apapirovski left a comment •

edited

Loading

benjamingr commented May 19, 2018 •

edited

Loading

jasnell commented May 19, 2018

BridgeAR commented May 19, 2018

jasnell commented May 19, 2018

addaleax commented May 19, 2018

TimothyGu Jun 26, 2018 •

edited

Loading

BridgeAR Jul 14, 2018

BridgeAR commented Jul 14, 2018 •

edited

Loading

TimothyGu commented Jul 15, 2018

BridgeAR commented Jul 16, 2018

BridgeAR commented Jul 16, 2018

targos commented Jul 16, 2018

lib,src: replace all C++ promises with JS promises #20830

lib,src: replace all C++ promises with JS promises #20830

Conversation

BridgeAR commented May 18, 2018

Checklist

BridgeAR commented May 19, 2018

TimothyGu commented May 19, 2018

BridgeAR commented May 19, 2018

TimothyGu commented May 19, 2018

BridgeAR commented May 19, 2018

TimothyGu commented May 19, 2018

devsnek commented May 19, 2018 • edited Loading

TimothyGu commented May 19, 2018

benjamingr commented May 19, 2018

benjamingr May 19, 2018

Choose a reason for hiding this comment

bmeurer May 19, 2018

Choose a reason for hiding this comment

benjamingr May 19, 2018

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

bmeurer left a comment

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

apapirovski left a comment • edited Loading

Choose a reason for hiding this comment

benjamingr commented May 19, 2018 • edited Loading

jasnell commented May 19, 2018

BridgeAR commented May 19, 2018

jasnell commented May 19, 2018

addaleax commented May 19, 2018

TimothyGu Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR Jul 14, 2018

Choose a reason for hiding this comment

BridgeAR commented Jul 14, 2018 • edited Loading

TimothyGu commented Jul 15, 2018

BridgeAR commented Jul 16, 2018

BridgeAR commented Jul 16, 2018

targos commented Jul 16, 2018

devsnek commented May 19, 2018 •

edited

Loading

apapirovski left a comment •

edited

Loading

benjamingr commented May 19, 2018 •

edited

Loading

TimothyGu Jun 26, 2018 •

edited

Loading

BridgeAR commented Jul 14, 2018 •

edited

Loading