GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: implement --trace-promises by joyeecheung · Pull Request #50899 · nodejs/node

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Nov 24, 2023

Copy link
Copy Markdown
Member

This patch implements a debugging flag that dumps the current stack trace when a promise is created or resolved. To reduce noise we ignore before and after events (as the execution is less interesting) and use the per-isolate hook to avoid extra JS frames. This flag can assist work in reducing unnecessary promise overhead.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
CanadaHonk, theanarkh, H4ad, and trivikr reacted with thumbs up emoji bricss, H4ad, and jazzypants1989 reacted with heart emoji
All reactions
  • 👍 4 reactions
  • ❤️ 3 reactions
@nodejs-github-bot

nodejs-github-bot commented Nov 24, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/startup
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Nov 24, 2023
@joyeecheung joyeecheung mentioned this pull request Nov 24, 2023
Reduce the promise overhead in the ESM loader #50900
Closed
GeoffreyBooth
GeoffreyBooth reviewed Nov 24, 2023
View reviewed changes

@GeoffreyBooth GeoffreyBooth left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This will be very useful! Just needs docs.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@anonrig anonrig added the notable-change PRs with changes that should be highlighted in changelogs. label Nov 24, 2023
@github-actions

github-actions Bot commented Nov 24, 2023

Copy link
Copy Markdown
Contributor

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @anonrig.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anonrig
anonrig approved these changes Nov 24, 2023
View reviewed changes

@anonrig anonrig left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you update docs as well?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/env.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Nov 24, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

This will be very useful! Just needs docs.

I think this should be a core developer debugging flag (we have several of those, like --verify-base-objects and --inspect-brk-node) so we should avoid documenting it. At least for now, lest some user depends on it. We can consider documenting it later when it's used a bit more internally. (I am not even sure if this is free of bugs, but as far as I've tested it works well enough).
GeoffreyBooth reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung removed the notable-change PRs with changes that should be highlighted in changelogs. label Nov 24, 2023
GeoffreyBooth
GeoffreyBooth approved these changes Nov 25, 2023
View reviewed changes
@theanarkh

theanarkh commented Nov 25, 2023

Copy link
Copy Markdown
Contributor

Does SetPromiseHook have any effect on performance ?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Nov 25, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Does SetPromiseHook have any effect on performance ?

I think the cost can be non-trivial, depending on how promise-heavy the use case is, although it’s just a debugging flag so it’s not supposed to be turned on in any case that care about performance.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
H4ad
H4ad approved these changes Nov 26, 2023
View reviewed changes
joyeecheung added 2 commits November 27, 2023 16:27
@joyeecheung
src: implement --trace-promises
58a83ff 
This patch implements a debugging flag that dumps the current
stack trace when a promise is created or resolved. To reduce
noise we ignore before and after events (as the execution is
less interesting) and use the per-isolate hook to avoid extra
JS frames. This flag can assist work in reducing unnecessary
promise overhead.
@joyeecheung
fixup! src: implement --trace-promises
668ab33
@joyeecheung
joyeecheung force-pushed the trace-promises branch from 27409f1 to 668ab33 Compare November 27, 2023 15:31
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 27, 2023
@joyeecheung

joyeecheung commented Nov 27, 2023

Copy link
Copy Markdown
Member Author

Removed it from the tests because we are intentionally not documenting it for now.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 27, 2023
@nodejs-github-bot

nodejs-github-bot commented Nov 27, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55956/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
legendecas
legendecas approved these changes Nov 27, 2023
View reviewed changes
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 27, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 27, 2023
@nodejs-github-bot

nodejs-github-bot commented Nov 27, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55958/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr

benjamingr commented Nov 27, 2023

Copy link
Copy Markdown
Member

Does SetPromiseHook have any effect on performance ?

Pretty significant but being set in C++ helps
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr approved these changes Nov 27, 2023
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Nov 28, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55979/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 28, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/55987/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Nov 30, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56019/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Dec 1, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56030/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 1, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 1, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 1, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56032/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Dec 3, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56052/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung added commit-queue PRs queued for automated landing through the Commit Queue. commit-queue-squash PRs the Commit Queue should land as one squashed commit. labels Dec 5, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 5, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 1e31a01 into nodejs:main Dec 5, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 5, 2023

Copy link
Copy Markdown
Collaborator

Landed in 1e31a01
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
RafaelGSS pushed a commit that referenced this pull request Dec 15, 2023
@joyeecheung @RafaelGSS
src: implement --trace-promises
c3abdc5 
This patch implements a debugging flag that dumps the current
stack trace when a promise is created or resolved. To reduce
noise we ignore before and after events (as the execution is
less interesting) and use the per-isolate hook to avoid extra
JS frames. This flag can assist work in reducing unnecessary
promise overhead.

PR-URL: #50899
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Dec 15, 2023
v21.5.0 proposal #51166
Merged
@H4ad H4ad mentioned this pull request Feb 22, 2024
docs: test for cli options being in sync between node_options.cc and cli.md #51623
Merged
@DanKaplanSES DanKaplanSES mentioned this pull request Mar 5, 2024
Print a "current stack/callback/promise dump" from a running program #51980
Closed
@DanKaplanSES

DanKaplanSES commented Mar 5, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

FYI, I created a feature request for a "promise dump" of a currently running program. I thought I'd mention it here because it might be another useful dev tool like --trace-promises, and to me it felt related. I'd love to hear others' thoughts.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@joyeecheung @richardlau
src: implement --trace-promises
c705b73 
This patch implements a debugging flag that dumps the current
stack trace when a promise is created or resolved. To reduce
noise we ignore before and after events (as the execution is
less interesting) and use the per-isolate hook to avoid extra
JS frames. This flag can assist work in reducing unnecessary
promise overhead.

PR-URL: #50899
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
v20.12.0 proposal #52212
Merged
This was referenced Aug 10, 2026
doc: --trace-promises is not documented in the CLI reference tomass22/node#2
Open
doc: document --trace-promises tomass22/node#1
Merged
doc: document --trace-promises #65199
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes
@benjamingr benjamingr benjamingr approved these changes
@anonrig anonrig anonrig approved these changes
@legendecas legendecas legendecas approved these changes
@H4ad H4ad H4ad approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash PRs the Commit Queue should land as one squashed commit. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@joyeecheung @nodejs-github-bot @theanarkh @benjamingr @DanKaplanSES @GeoffreyBooth @anonrig @legendecas @H4ad
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.