Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: use new v8::Context PromiseHook API #36394

Closed
wants to merge 6 commits into from

Conversation

Qard
Copy link
Member

@Qard Qard commented Dec 4, 2020

I've been working on an alternative to the existing PromiseHook API in V8 using split JSFunctions per event type rather than the old way which routes everything through a single C++ function even though it's just going right back to JS in the end. The old way has many, many costly boundary jumps and deopts in many cases, so we really need a better way to track promise lifecycle events. 😅

The corresponding Chromium Review is here: https://chromium-review.googlesource.com/c/v8/v8/+/2466783 https://chromium-review.googlesource.com/c/v8/v8/+/2759188

As the change has not yet landed in V8, I'm opening this as a draft PR just to show my progress and give people something to look at. I've got some nice benchmarks to share too!

Before:

❯ ./node-master benchmark/async_hooks/promises.js
async_hooks/promises.js asyncHooks="enabled" n=1000000: 271,514.4761845998
async_hooks/promises.js asyncHooks="enabledWithDestroy" n=1000000: 187,109.67499761964
async_hooks/promises.js asyncHooks="enabledWithInitOnly" n=1000000: 315,364.83307747904
async_hooks/promises.js asyncHooks="disabled" n=1000000: 1,668,525.7547478643

After:

❯ ./node benchmark/async_hooks/promises.js       
async_hooks/promises.js asyncHooks="enabled" n=1000000: 1,250,448.989340235
async_hooks/promises.js asyncHooks="enabledWithDestroy" n=1000000: 194,382.33020796857
async_hooks/promises.js asyncHooks="enabledWithInitOnly" n=1000000: 1,071,082.1953050198
async_hooks/promises.js asyncHooks="disabled" n=1000000: 1,673,424.8290965108

It's getting about 3-4x better performance. Not quite on-par with disabled yet, but getting close. There is still much that can be done to improve those numbers though.

The current changes do not allow the optimizer to do its magic in certain cases so I'm hoping to dig a bit more into getting those cases to optimize too. For reference though, the existing PromiseHook API bails out too, so this is quite a bit faster already. I'm also investigating moving the PromiseWrap used when there's a destroy hook to using this model and wrapping from JS instead, which I think should still be an overall performance improvement despite the hop back to native for the initial wrap.

cc @nodejs/diagnostics @nodejs/v8

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@Qard Qard added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. performance Issues and PRs related to the performance of Node.js. async_wrap async_hooks Issues and PRs related to the async hooks subsystem. diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. labels Dec 4, 2020
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 4, 2020
@Qard Qard force-pushed the jsfunction-promise-hook branch 2 times, most recently from fb63787 to 6567930 Compare December 4, 2020 20:31
@AndreasMadsen
Copy link
Member

This is amazing work. I still think nodejs/diagnostics#389 is the best long-term solution, but regardless there will always be a huge value in being able to fully track promises.

@mhdawson
Copy link
Member

mhdawson commented Dec 9, 2020

@Qard looks very promising.

@Qard
Copy link
Member Author

Qard commented Dec 9, 2020

@AndreasMadsen That can actually be fully achieved on the consuming end of this PromiseHook API, so we can always try that later. Because it's a major breaking change though, I don't want to mix that in here. For the time being, my goal is to just make what already works faster. 😸

trackPromise(promise, parent);
const asyncId = promise[async_id_symbol];
const triggerAsyncId = promise[trigger_async_id_symbol];
emitInitScript(asyncId, 'PROMISE', triggerAsyncId, promise);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has previously been discussed to not expose the actual promise object via async_hooks (and eventually PromiseHooks), in the hope that one day we could enable escape analysis for promise objects. See #23443 and nodejs/diagnostics#188 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That only applies when using PromiseWrap though, which is quite expensive and therefore I've been working towards removing it. Also, AsyncLocalStorage requires the promise, or a distinct resource with the same lifetime as the promise itself, to work properly so there's not currently a way to eliminate that. I'm happy to hear any other suggestions you can think of to handle that.

I've been doing this work, and my prior async_hooks work this year, based on that PromiseHook proposal doc and the later discussions at the summit in Munich. I went for a slightly different compromise path which achieves mostly all the performance wins available in those ideas while avoiding significant breaking changes. This will (hopefully) allow the changes to be backported. We can then consider the remaining breaking parts later. The one part I do still intend to look into from that doc in the short-term is if I can replace PromiseWrap gc-tracking with WeakRef finalizers instead.

Local<Value> after_hook = args[2];
Local<Value> resolve_hook = args[3];
ctx->SetPromiseHooks(init_hook, before_hook, after_hook, resolve_hook);
}

static void DisablePromiseHook(const FunctionCallbackInfo<Value>& args) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EnablePromiseHook()/DisablePromiseHook() still suffer from the problem mentioned in the design doc that they perform per-Isolate actions, when what we want are per-Context actions… we could make that go away with the new hook functions, though, right? We’d just need to make the init_hook function here do a call into C++ to set up the GC tracking if there any destroy hooks, correct?

Copy link
Member Author

@Qard Qard Dec 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the destroy-hook fallback remains unchanged currently, so it still suffers from the isolate issues. I'm hoping to eliminate that eventually, but it may have to wait until later. My plan is to eliminate the switch to native hooks for destroy hooks and instead just apply gc-tracking from the JS side. Might make the gc-tracking a bit more expensive, but I suspect the benefit of the rest of the hooks moving to JS should more than offset that.

@Qard
Copy link
Member Author

Qard commented Feb 22, 2021

I'll be getting back to this starting tomorrow. Just been taking some time off from code. 😅

@Qard
Copy link
Member Author

Qard commented Feb 28, 2021

I've updated with the latest V8 changes. As far as I'm aware the V8 stuff should be ready to land now, just waiting on approval. Once that gets in I can make a proper backport commit for the V8 change and then this should be safe to land on top of that. :)

@Qard
Copy link
Member Author

Qard commented Mar 22, 2021

The V8 changes have moved to https://chromium-review.googlesource.com/c/v8/v8/+/2759188. Hopefully I can get that landed soon. 😅

foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
🤦 Might help if I remember to disable the _other_ promise
hook implementation when switching between them...

Fixes nodejs#38814
Fixes nodejs#38815
Refs nodejs#36394

PR-URL: nodejs#38912
Backport-PR-URL: nodejs#38577
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
@Qard Qard deleted the jsfunction-promise-hook branch June 18, 2022 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. lib / src Issues and PRs related to general changes in the lib or src directory. performance Issues and PRs related to the performance of Node.js. semver-minor PRs that contain new features and should be released in the next minor version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.