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

Set force-frame-pointers=yes on arm machines on arm linux #557

Merged
merged 1 commit into from
Jul 6, 2023
Merged

Set force-frame-pointers=yes on arm machines on arm linux #557

merged 1 commit into from
Jul 6, 2023

Conversation

jackkleeman
Copy link
Contributor

To enable Parca - parca-dev/parca-agent#1805

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR @jackkleeman. Before merging it, what are the implications for Restate when enabling the frame pointers (like why isn't it on by default)?

@jackkleeman
Copy link
Contributor Author

On a few platform eg apple (rust-lang/rust#85706) and openbsd the frame pointers are actually on by default. This field influences LLVM compiler config, which would otherwise only include frame pointers for some functions based on some factors eg whether they have variable size allocations:
https://github.com/llvm/llvm-project/blob/c0318f079a0c7a0558c29f04b4e190277f6f6687/llvm/lib/Target/X86/X86FrameLowering.cpp#L97

This discusses the perf implications of enabling frame pointers:
https://stackoverflow.com/questions/13006371/does-omitting-the-frame-pointers-really-have-a-positive-effect-on-performance-an
It seems perhaps some applications see a 5-10% slowdown.
This comment says that rustc and ripgrep saw negligible slowdowns:
rust-lang/rust#86196 (comment)

I can change to just enabling this for arm, given thats where we need it (and it already will be enabled on apple targets, so really the delta is just AWS arm servers). I also suspect we don't need this forever, as parca will probably work on getting the same info without frame pointers.

@jackkleeman jackkleeman changed the title Set force-frame-pointers=yes Set force-frame-pointers=yes on arm machines on arm linux Jul 5, 2023
@tillrohrmann
Copy link
Contributor

Thanks for the pointers @jackkleeman. I will try to see whether I can measure a difference in performance on arm64 on Linux. If not, then this PR should be good to be merged.

Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

When testing it locally in my Docker environment, I couldn't measure a significant change in performance. Hence, +1 for merging it.

@jackkleeman jackkleeman merged commit d7ca3fe into restatedev:main Jul 6, 2023
@jackkleeman jackkleeman deleted the frame-pointers branch July 6, 2023 10:19
@slinkydeveloper
Copy link
Contributor

@jackkleeman Have you checked we don't need to build the std library with nightly to enable frame pointers? I've seen this around: https://docs.rs/tracefp/latest/tracefp/

@jackkleeman
Copy link
Contributor Author

I don't believe so, no. Let's see if Parca works with this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants