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

src: implement --trace-promises #50899

Merged
merged 2 commits into from
Dec 5, 2023

Conversation

joyeecheung
Copy link
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.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/startup

@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 related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Nov 24, 2023
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a 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.

@anonrig anonrig added the notable-change PRs with changes that should be highlighted in changelogs. label Nov 24, 2023
Copy link
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.

Copy link
Member

@anonrig anonrig left a 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?

src/env.cc Show resolved Hide resolved
@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 24, 2023

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).

@joyeecheung joyeecheung removed the notable-change PRs with changes that should be highlighted in changelogs. label Nov 24, 2023
@theanarkh
Copy link
Contributor

Does SetPromiseHook have any effect on performance ?

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 25, 2023

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.

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 joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2023
@joyeecheung
Copy link
Member Author

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

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2023
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2023
@nodejs-github-bot
Copy link
Collaborator

@benjamingr
Copy link
Member

Does SetPromiseHook have any effect on performance ?

Pretty significant but being set in C++ helps

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 1, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 1, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Dec 5, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 5, 2023
@nodejs-github-bot nodejs-github-bot merged commit 1e31a01 into nodejs:main Dec 5, 2023
62 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 1e31a01

RafaelGSS pushed a commit that referenced this pull request Dec 15, 2023
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 <yagiz.nizipli@sentry.io>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Dec 15, 2023
@DanKaplanSES
Copy link
Contributor

DanKaplanSES commented Mar 5, 2024

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.

richardlau pushed a commit that referenced this pull request Mar 25, 2024
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 <yagiz.nizipli@sentry.io>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants