Home Original page

[v14.x backport] async_hooks: use new v8::Context PromiseHook API by Qard · Pull Request #38577 · nodejs/node

@Qard added c++

Issues and PRs that require attention from people who are familiar with C++.

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.

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.

async_wrap async_hooks

Issues and PRs related to the async hooks subsystem.

labels

May 7, 2021

vdeturckheim

@Qard Qard added the author ready

PRs that have at least one approval, no pending requests for changes, and a CI started.

label

May 7, 2021

Stephen Belanger and others added 7 commits

August 1, 2021 12:02
Original commit message:

    [promises] Change context promise hooks to Callable

    The previously added perf-context Promise-hooks take a v8::Function as
    arguments. However, the builtin code was only accepting JSFunctions
    which causes cast errors.

    Drive-by-fix: Directly pass nativeContext in more places.

    Bug: chromium:1201465
    Change-Id: Ic8bed11253a1f18a84e71eb9ea809b1ec1c3f428
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2850162
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74223}

Refs: v8/v8@5f44131

PR-URL: nodejs#36394
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Original commit message:

    [promises] Fix slow path when context promise hooks are present

    Bug: chromium:1201936
    Change-Id: I1ee545e33587ddf4a5c7e1cbd64b53d36c75a146
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2850936
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74267}

Refs: v8/v8@4c07451

PR-URL: nodejs#36394
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Original commit message:

    [runtime] Fix Promise.all context promise hooks

    We have to take the slow path in Promise.all if context promise hooks
    are set. The fast-path doesn't create intermediate promises by default.

    Bug: chromium:1204132, v8:11025
    Change-Id: Ide92de00a4f6df05e0ddbc8814f6673bd667f426
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2866771
    Reviewed-by: Victor Gomes <victorgomes@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74326}

Refs: v8/v8@fa4cb17
PR-URL: nodejs#36394
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
The new JS PromiseHooks introduced in the referenced PR are per
v8::Context. This meant that code depending on them, such as
AsyncLocalStorage, wouldn't behave correctly across vm.Context
instances.

PromiseHooks are now synchronized across the main Context and any
Context created via vm.Context.

Refs: nodejs#36394
Fixes: nodejs#38781
Signed-off-by: Bryan English <bryan@bryanenglish.com>

PR-URL: nodejs#38821
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
🤦 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
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
This way we don't end up attempting to SetPromiseHooks on contexts that
have already been collected.

Fixes: nodejs#39019

PR-URL: nodejs#39095
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>

@Qard Qard added the request-ci

Add this label to start a Jenkins CI on a PR.

label

Aug 1, 2021

@Qard Qard added request-ci

Add this label to start a Jenkins CI on a PR.

and removed needs-ci

PRs that need a full CI run.

labels

Aug 1, 2021

targos pushed a commit that referenced this pull request

Aug 3, 2021 
Original commit message:

    Reland "[api] JSFunction PromiseHook for v8::Context"

    This is a reland of d5457f5fb7ea05ca05a697599ffa50d35c1ae3c7
    after a speculative revert.

    Additionally it fixes an issue with throwing promise hooks.

    Original change's description:
    > [api] JSFunction PromiseHook for v8::Context
    >
    > This will enable Node.js to get much better performance from async_hooks
    > as currently PromiseHook delegates to C++ for the hook function and then
    > Node.js delegates it right back to JavaScript, introducing several
    > unnecessary barrier hops in code that gets called very, very frequently
    > in modern, promise-heavy applications.
    >
    > This API mirrors the form of the original C++ function based PromiseHook
    > API, however it is intentionally separate to allow it to use JSFunctions
    > triggered within generated code to, as much as possible, avoid entering
    > runtime functions entirely.
    >
    > Because PromiseHook has internal use also, beyond just the Node.js use,
    > I have opted to leave the existing API intact and keep this separate to
    > avoid conflicting with any possible behaviour expectations of other API
    > users.
    >
    > The design ideas for this new API stemmed from discussion with some V8
    > team members at a previous Node.js Diagnostics Summit hosted by Google
    > in Munich, and the relevant documentation of the discussion can be found
    > here: https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/edit#heading=h.w1bavzz80l1e
    >
    > A summary of the reasons for why this new design is important can be
    > found here: https://docs.google.com/document/d/1vtgoT4_kjgOr-Bl605HR2T6_SC-C8uWzYaOPDK5pmRo/edit?usp=sharing
    >
    > Bug: v8:11025
    > Change-Id: I0b403b00c37d3020b5af07b654b860659d3a7697
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2759188
    > Reviewed-by: Marja Hölttä <marja@chromium.org>
    > Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    > Reviewed-by: Anton Bikineev <bikineev@chromium.org>
    > Reviewed-by: Igor Sheludko <ishell@chromium.org>
    > Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    > Cr-Commit-Position: refs/heads/master@{#73858}

    Bug: v8:11025
    Bug: chromium:1197475
    Change-Id: I73a71e97d9c3dff89a2b092c3fe4adff81ede8ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2823917
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Anton Bikineev <bikineev@chromium.org>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74071}

Refs: v8/v8@c0fceaa

PR-URL: #36394
Backport-PR-URL: #38577
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request

Aug 3, 2021 
Original commit message:

    [runtime] Fix promise hooks

    promiseCapability can be undefined.

    Bug: v8:11025
    Bug: chromium:1201113
    Change-Id: I9da8764820cee0db1f0c38ed2fff0e3afeb9a80e
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2844649
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74117}

Refs: v8/v8@272445f

PR-URL: #36394
Backport-PR-URL: #38577
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request

Aug 3, 2021 
Original commit message:

    [promises] Change context promise hooks to Callable

    The previously added perf-context Promise-hooks take a v8::Function as
    arguments. However, the builtin code was only accepting JSFunctions
    which causes cast errors.

    Drive-by-fix: Directly pass nativeContext in more places.

    Bug: chromium:1201465
    Change-Id: Ic8bed11253a1f18a84e71eb9ea809b1ec1c3f428
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2850162
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74223}

Refs: v8/v8@5f44131

PR-URL: #36394
Backport-PR-URL: #38577
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request

Aug 3, 2021 
Original commit message:

    [promises] Fix slow path when context promise hooks are present

    Bug: chromium:1201936
    Change-Id: I1ee545e33587ddf4a5c7e1cbd64b53d36c75a146
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2850936
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74267}

Refs: v8/v8@4c07451

PR-URL: #36394
Backport-PR-URL: #38577
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request

Aug 3, 2021 
Original commit message:

    [runtime] Fix Promise.all context promise hooks

    We have to take the slow path in Promise.all if context promise hooks
    are set. The fast-path doesn't create intermediate promises by default.

    Bug: chromium:1204132, v8:11025
    Change-Id: Ide92de00a4f6df05e0ddbc8814f6673bd667f426
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2866771
    Reviewed-by: Victor Gomes <victorgomes@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74326}

Refs: v8/v8@fa4cb17

PR-URL: #38577
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

targos pushed a commit that referenced this pull request

Aug 3, 2021 
PR-URL: #36394
Backport-PR-URL: #38577
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

targos pushed a commit that referenced this pull request

Aug 3, 2021 
The new JS PromiseHooks introduced in the referenced PR are per
v8::Context. This meant that code depending on them, such as
AsyncLocalStorage, wouldn't behave correctly across vm.Context
instances.

PromiseHooks are now synchronized across the main Context and any
Context created via vm.Context.

Refs: #36394
Fixes: #38781
Signed-off-by: Bryan English <bryan@bryanenglish.com>

PR-URL: #38821
Backport-PR-URL: #38577
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

targos pushed a commit that referenced this pull request

Aug 3, 2021 
🤦 Might help if I remember to disable the _other_ promise
hook implementation when switching between them...

Fixes #38814
Fixes #38815
Refs #36394

PR-URL: #38912
Backport-PR-URL: #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>

targos pushed a commit that referenced this pull request

Aug 3, 2021 
This way we don't end up attempting to SetPromiseHooks on contexts that
have already been collected.

Fixes: #39019

PR-URL: #39095
Backport-PR-URL: #38577
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>

@Qard Qard deleted the backport-36394-to-14 branch

August 3, 2021 15:11

BethGriggs pushed a commit that referenced this pull request

Aug 12, 2021 
Original commit message:

    Reland "[api] JSFunction PromiseHook for v8::Context"

    This is a reland of d5457f5fb7ea05ca05a697599ffa50d35c1ae3c7
    after a speculative revert.

    Additionally it fixes an issue with throwing promise hooks.

    Original change's description:
    > [api] JSFunction PromiseHook for v8::Context
    >
    > This will enable Node.js to get much better performance from async_hooks
    > as currently PromiseHook delegates to C++ for the hook function and then
    > Node.js delegates it right back to JavaScript, introducing several
    > unnecessary barrier hops in code that gets called very, very frequently
    > in modern, promise-heavy applications.
    >
    > This API mirrors the form of the original C++ function based PromiseHook
    > API, however it is intentionally separate to allow it to use JSFunctions
    > triggered within generated code to, as much as possible, avoid entering
    > runtime functions entirely.
    >
    > Because PromiseHook has internal use also, beyond just the Node.js use,
    > I have opted to leave the existing API intact and keep this separate to
    > avoid conflicting with any possible behaviour expectations of other API
    > users.
    >
    > The design ideas for this new API stemmed from discussion with some V8
    > team members at a previous Node.js Diagnostics Summit hosted by Google
    > in Munich, and the relevant documentation of the discussion can be found
    > here: https://docs.google.com/document/d/1g8OrG5lMIUhRn1zbkutgY83MiTSMx-0NHDs8Bf-nXxM/edit#heading=h.w1bavzz80l1e
    >
    > A summary of the reasons for why this new design is important can be
    > found here: https://docs.google.com/document/d/1vtgoT4_kjgOr-Bl605HR2T6_SC-C8uWzYaOPDK5pmRo/edit?usp=sharing
    >
    > Bug: v8:11025
    > Change-Id: I0b403b00c37d3020b5af07b654b860659d3a7697
    > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2759188
    > Reviewed-by: Marja Hölttä <marja@chromium.org>
    > Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    > Reviewed-by: Anton Bikineev <bikineev@chromium.org>
    > Reviewed-by: Igor Sheludko <ishell@chromium.org>
    > Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    > Cr-Commit-Position: refs/heads/master@{#73858}

    Bug: v8:11025
    Bug: chromium:1197475
    Change-Id: I73a71e97d9c3dff89a2b092c3fe4adff81ede8ef
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2823917
    Reviewed-by: Marja Hölttä <marja@chromium.org>
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Reviewed-by: Anton Bikineev <bikineev@chromium.org>
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Commit-Queue: Camillo Bruni <cbruni@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#74071}

Refs: v8/v8@c0fceaa

PR-URL: #36394
Backport-PR-URL: #38577
Reviewed-By: Bryan English <bryan@bryanenglish.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: James M Snell <jasnell@gmail.com>