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

Qard · 2021-05-07T01:56:40Z

This is a backport of #36394 to v14. There are some notable differences due to the older V8 version not having the newer ContextSlot enum with type-safe slots, but other than that is basically identical.

nodejs-github-bot · 2021-05-07T02:05:34Z

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

Qard · 2021-05-07T15:53:03Z

The GitHub Actions linux test is failing consistently with an error from buffers about something being used uninitialized. I don't see how that would be related to my changes. Is this a known issue in the v14.x-staging branch?

../src/node_buffer.cc:665:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  665 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc:662:10: error: ‘max_length’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  662 |   size_t max_length;
      |          ^~~~~~~~~~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::ASCII]’:
../src/node_buffer.cc:665:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  665 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc:662:10: error: ‘max_length’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  662 |   size_t max_length;
      |          ^~~~~~~~~~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::BASE64]’:
../src/node_buffer.cc:665:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  665 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::BINARY]’:
../src/node_buffer.cc:665:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  665 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::HEX]’:
../src/node_buffer.cc:665:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  665 |   if (offset > ts_obj_length) {
      |   ^~
../src/node_buffer.cc:662:10: error: ‘max_length’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  662 |   size_t max_length;
      |          ^~~~~~~~~~
../src/node_buffer.cc: In function ‘void node::Buffer::{anonymous}::StringWrite(const v8::FunctionCallbackInfo<v8::Value>&) [with node::encoding encoding = node::UCS2]’:
../src/node_buffer.cc:665:3: error: ‘offset’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  665 |   if (offset > ts_obj_length) {
      |   ^~

Qard · 2021-05-07T16:04:02Z

I see the latest commits on the branch have that failure too, I guess that is just getting ignored and PRs landing anyway. 🤷

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 <[email protected]> Commit-Queue: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#74223} Refs: v8/v8@5f44131 PR-URL: nodejs#36394 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]>

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ä <[email protected]> Reviewed-by: Jakob Gruber <[email protected]> Commit-Queue: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#74267} Refs: v8/v8@4c07451 PR-URL: nodejs#36394 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Commit-Queue: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#74326} Refs: v8/v8@fa4cb17

PR-URL: nodejs#36394 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> PR-URL: nodejs#38821 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

🤦 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 <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Bryan English <[email protected]>

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 <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>

nodejs-github-bot · 2021-08-01T19:13:17Z

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

nodejs-github-bot · 2021-08-01T22:29:58Z

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

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ä <[email protected]> > Reviewed-by: Camillo Bruni <[email protected]> > Reviewed-by: Anton Bikineev <[email protected]> > Reviewed-by: Igor Sheludko <[email protected]> > Commit-Queue: Camillo Bruni <[email protected]> > 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ä <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Reviewed-by: Anton Bikineev <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Commit-Queue: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#74071} Refs: v8/v8@c0fceaa PR-URL: #36394 Backport-PR-URL: #38577 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]>

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ä <[email protected]> Commit-Queue: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#74117} Refs: v8/v8@272445f PR-URL: #36394 Backport-PR-URL: #38577 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Commit-Queue: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#74223} Refs: v8/v8@5f44131 PR-URL: #36394 Backport-PR-URL: #38577 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]>

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ä <[email protected]> Reviewed-by: Jakob Gruber <[email protected]> Commit-Queue: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#74267} Refs: v8/v8@4c07451 PR-URL: #36394 Backport-PR-URL: #38577 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Commit-Queue: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#74326} Refs: v8/v8@fa4cb17 PR-URL: #38577 Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Bryan English <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: #36394 Backport-PR-URL: #38577 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> PR-URL: #38821 Backport-PR-URL: #38577 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

🤦 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 <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Bryan English <[email protected]>

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 <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>

targos · 2021-08-03T08:27:30Z

Landed in 0061e5d...a986158

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ä <[email protected]> > Reviewed-by: Camillo Bruni <[email protected]> > Reviewed-by: Anton Bikineev <[email protected]> > Reviewed-by: Igor Sheludko <[email protected]> > Commit-Queue: Camillo Bruni <[email protected]> > 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ä <[email protected]> Reviewed-by: Igor Sheludko <[email protected]> Reviewed-by: Anton Bikineev <[email protected]> Reviewed-by: Camillo Bruni <[email protected]> Commit-Queue: Camillo Bruni <[email protected]> Cr-Commit-Position: refs/heads/master@{#74071} Refs: v8/v8@c0fceaa PR-URL: #36394 Backport-PR-URL: #38577 Reviewed-By: Bryan English <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: James M Snell <[email protected]>

Qard requested review from Flarna, bengl, devsnek, jasnell and vdeturckheim May 7, 2021 01:56

github-actions bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v14.x labels May 7, 2021

Qard added backported-to-v14.x and removed build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v14.x labels May 7, 2021

Qard force-pushed the backport-36394-to-14 branch from 69bb9b8 to c5e277d Compare May 7, 2021 02:03

Qard added needs-ci PRs that need a full CI run. v14.x and removed backported-to-v14.x labels May 7, 2021

Flarna approved these changes May 7, 2021

View reviewed changes

vdeturckheim approved these changes May 7, 2021

View reviewed changes

bengl approved these changes May 7, 2021

View reviewed changes

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

Qard force-pushed the backport-36394-to-14 branch from 116c0ba to 3e2fdff Compare August 1, 2021 19:03

Qard added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2021

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2021

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

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 1, 2021

targos closed this Aug 3, 2021

Qard deleted the backport-36394-to-14 branch August 3, 2021 15:11

targos mentioned this pull request Aug 25, 2021

[v14.x backport] deps: V8: cherry-pick 81814ed44574 #39743

Closed

targos mentioned this pull request Sep 4, 2021

v14.18.0 release proposal #39990

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[v14.x backport] async_hooks: use new v8::Context PromiseHook API#38577

[v14.x backport] async_hooks: use new v8::Context PromiseHook API#38577
Qard wants to merge 9 commits intonodejs:v14.x-stagingfrom
Qard:backport-36394-to-14

Qard commented May 7, 2021

Uh oh!

nodejs-github-bot commented May 7, 2021

Uh oh!

Qard commented May 7, 2021

Uh oh!

Qard commented May 7, 2021

Uh oh!

nodejs-github-bot commented Aug 1, 2021

Uh oh!

nodejs-github-bot commented Aug 1, 2021

Uh oh!

targos commented Aug 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Uh oh!

Conversation

Qard commented May 7, 2021

Uh oh!

nodejs-github-bot commented May 7, 2021

Uh oh!

Qard commented May 7, 2021

Uh oh!

Qard commented May 7, 2021

Uh oh!

nodejs-github-bot commented Aug 1, 2021

Uh oh!

nodejs-github-bot commented Aug 1, 2021

Uh oh!

targos commented Aug 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants