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

build: go faster, drop -fno-omit-frame-pointer #44452

Merged

Conversation

bnoordhuis
Copy link
Member

This flag was added back in 2013 to support postmortem debugging on
Linux but it has a pretty bad impact on performance (up to 10%) and
I don't think it's actually necessary to get meaningful stack traces.
Maybe with mdb but gdb and lldb seem to manage just fine.

Even if the flag is necessary for postmortem debugging, I don't think
it's appropriate to make performance for all users suffer for the
benefit of a fringe group.

Signed, a member of said fringe group.

This flag was added back in 2013 to support postmortem debugging on
Linux but it has a pretty bad impact on performance (up to 10%) and
I don't think it's actually necessary to get meaningful stack traces.
Maybe with mdb but gdb and lldb seem to manage just fine.

Even if the flag is necessary for postmortem debugging, I don't think
it's appropriate to make performance for all users suffer for the
benefit of a fringe group.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Aug 30, 2022
@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 31, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 31, 2022
@nodejs-github-bot
Copy link
Collaborator

@gengjiawen gengjiawen added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 2, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 2, 2022
@nodejs-github-bot nodejs-github-bot merged commit d0f73d3 into nodejs:main Sep 2, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in d0f73d3

RafaelGSS pushed a commit that referenced this pull request Sep 5, 2022
This flag was added back in 2013 to support postmortem debugging on
Linux but it has a pretty bad impact on performance (up to 10%) and
I don't think it's actually necessary to get meaningful stack traces.
Maybe with mdb but gdb and lldb seem to manage just fine.

Even if the flag is necessary for postmortem debugging, I don't think
it's appropriate to make performance for all users suffer for the
benefit of a fringe group.

PR-URL: #44452
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 6, 2022
This flag was added back in 2013 to support postmortem debugging on
Linux but it has a pretty bad impact on performance (up to 10%) and
I don't think it's actually necessary to get meaningful stack traces.
Maybe with mdb but gdb and lldb seem to manage just fine.

Even if the flag is necessary for postmortem debugging, I don't think
it's appropriate to make performance for all users suffer for the
benefit of a fringe group.

PR-URL: #44452
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Sep 7, 2022
This flag was added back in 2013 to support postmortem debugging on
Linux but it has a pretty bad impact on performance (up to 10%) and
I don't think it's actually necessary to get meaningful stack traces.
Maybe with mdb but gdb and lldb seem to manage just fine.

Even if the flag is necessary for postmortem debugging, I don't think
it's appropriate to make performance for all users suffer for the
benefit of a fringe group.

PR-URL: #44452
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@RafaelGSS RafaelGSS added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 7, 2022
@RafaelGSS
Copy link
Member

RafaelGSS commented Sep 7, 2022

@bnoordhuis Sorry, I had to drop this commit from v18.9.0 release. This is making the linux-perf tests fail. See: https://ci.nodejs.org/job/node-test-commit-v8-linux/4847/

@bnoordhuis bnoordhuis deleted the no-fno-omit-frame-pointer branch September 7, 2022 21:06
@bnoordhuis
Copy link
Member Author

I'll open a revert. This is indeed caused by perf getting confused around C++ <-> JS transitions because there are no longer frame pointers to follow. perf --call-graph dwarf unfortunately doesn't help.

Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
This flag was added back in 2013 to support postmortem debugging on
Linux but it has a pretty bad impact on performance (up to 10%) and
I don't think it's actually necessary to get meaningful stack traces.
Maybe with mdb but gdb and lldb seem to manage just fine.

Even if the flag is necessary for postmortem debugging, I don't think
it's appropriate to make performance for all users suffer for the
benefit of a fringe group.

PR-URL: nodejs#44452
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@Flarna Flarna removed the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Oct 19, 2022
@Flarna
Copy link
Member

Flarna commented Oct 19, 2022

removed the backport-requested-v18.x label because this was reverted via #44566 therefore I doubt we want it on v18.

@Flarna Flarna added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v14.x labels Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants