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

Update build advice for benchmarking and profiling #22

Merged
merged 2 commits into from
Sep 23, 2024
Merged

Update build advice for benchmarking and profiling #22

merged 2 commits into from
Sep 23, 2024

Conversation

delan
Copy link
Member

@delan delan commented Sep 13, 2024

These patches update the Profiling and Building Servo chapters with new advice around building Servo for benchmarking and profiling purposes. In particular:

We also fix a typo, --with-frame-pointers--with-frame-pointer. That said, we should really enable stack frame pointers by default in all builds, because it’s now considered best practice.

(@atbrakhi, @jschwe)

@delan delan changed the title Profiling: update recommended build instructions Update build advice for benchmarking and profiling Sep 20, 2024
@delan delan marked this pull request as ready for review September 20, 2024 09:47
@jschwe
Copy link
Member

jschwe commented Sep 20, 2024

That said, we should really enable stack frame pointers by default in all builds, because it’s now considered best practice.

Yes, please! Although perhaps the default is already "On" for rust code (rust-lang/rust#122646 (comment)). I didn't check yet what the defaults for framepointers in user code are on the targets we support.

@delan
Copy link
Member Author

delan commented Sep 23, 2024

Yes, please! Although perhaps the default is already "On" for rust code (rust-lang/rust#122646 (comment)). I didn't check yet what the defaults for framepointers in user code are on the targets we support.

Regarding user code, it seems they are not yet forced by default on all platforms. I checked this by finding related patches (rust-lang/rust#61675, rust-lang/rust#86652, rust-lang/rust#107689, rust-lang/rust#114323, rust-lang/rust#115521, rust-lang/rust#124733) and seeing where the defaults are specified:

~/code/rust $ git log -n 1 --pretty=oneline
702987f75b74f789ba227ee04a3d7bb1680c2309 (HEAD -> master, origin/master, origin/HEAD) Auto merge of #130732 - matthiaskrgr:rollup-ke1j314, r=matthiaskrgr
~/code/rust $ rg frame_pointer: compiler/rustc_target/src/spec/mod.rs
2169:    pub frame_pointer: FramePointer,
2564:            frame_pointer: FramePointer::MayOmit,

~/code/rust $ rg --sort=path frame_pointer compiler/rustc_target/src/spec/targets | cat
compiler/rustc_target/src/spec/targets/aarch64_apple_darwin.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/aarch64_apple_ios.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/aarch64_apple_ios_macabi.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/aarch64_apple_ios_sim.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/aarch64_apple_tvos.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/aarch64_apple_tvos_sim.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/aarch64_apple_visionos.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/aarch64_apple_visionos_sim.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/aarch64_apple_watchos_sim.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/arm64e_apple_darwin.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/arm64e_apple_ios.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/arm64e_apple_tvos.rs:            frame_pointer: FramePointer::NonLeaf,
compiler/rustc_target/src/spec/targets/armv5te_none_eabi.rs:            frame_pointer: FramePointer::MayOmit,
compiler/rustc_target/src/spec/targets/i686_apple_darwin.rs:            frame_pointer: FramePointer::Always,
compiler/rustc_target/src/spec/targets/i686_pc_windows_gnu.rs:    base.frame_pointer = FramePointer::Always; // Required for backtraces
compiler/rustc_target/src/spec/targets/i686_pc_windows_gnullvm.rs:    base.frame_pointer = FramePointer::Always; // Required for backtraces
compiler/rustc_target/src/spec/targets/i686_unknown_linux_musl.rs:    base.frame_pointer = FramePointer::Always;
compiler/rustc_target/src/spec/targets/i686_uwp_windows_gnu.rs:    base.frame_pointer = FramePointer::Always; // Required for backtraces
compiler/rustc_target/src/spec/targets/thumbv4t_none_eabi.rs:            frame_pointer: FramePointer::MayOmit,
compiler/rustc_target/src/spec/targets/thumbv5te_none_eabi.rs:            frame_pointer: FramePointer::MayOmit,
compiler/rustc_target/src/spec/targets/x86_64_apple_darwin.rs:            frame_pointer: FramePointer::Always,
compiler/rustc_target/src/spec/targets/x86_64h_apple_darwin.rs:    opts.frame_pointer = FramePointer::Always;

I also confirmed this in our own builds for Linux on amd64. Most functions have frame pointers, but not all:

~/code/servo $ git log -n 1 --pretty=oneline
d3d6a22d27df5095c3342249d0eea0bce153cbe1 (HEAD -> main, upstream/main) ohos: Add back and fwd button to vendored app (#33511)
~/code/servo $ ./mach build --profile profiling
~/code/servo $ objdump -dSM intel target/profiling/servo | pv > objdump_profiling.txt
~/code/servo $ fgrep -A9 '_ZN67_$LT$layout_2020..dom..BoxSlot$u20$as$u20$core..ops..drop..Drop$GT$4drop17h03488a18e530c6c8E' objdump_profiling.txt
0000000000cf8e90 <_ZN67_$LT$layout_2020..dom..BoxSlot$u20$as$u20$core..ops..drop..Drop$GT$4drop17h03488a18e530c6c8E>:
}

impl Drop for BoxSlot<'_> {
    fn drop(&mut self) {
  cf8e90:	53                   	push   rbx
  cf8e91:	48 83 ec 50          	sub    rsp,0x50
  cf8e95:	48 8d 05 2c 2e c2 05 	lea    rax,[rip+0x5c22e2c]        # 691bcc8 <_ZN3std9panicking11panic_count18GLOBAL_PANIC_COUNT17hd7fda31a6dd5a9d3E>
  cf8e9c:	48 8b 00             	mov    rax,QWORD PTR [rax]
  cf8e9f:	48 d1 e0             	shl    rax,1
~/code/servo $ ./mach build --profile profiling --with-frame-pointer
~/code/servo $ objdump -dSM intel target/profiling/servo | pv > objdump_profiling_wfp.txt
~/code/servo $ fgrep -A9 '_ZN67_$LT$layout_2020..dom..BoxSlot$u20$as$u20$core..ops..drop..Drop$GT$4drop17h03488a18e530c6c8E' objdump_profiling_wfp.txt
0000000000cf03d0 <_ZN67_$LT$layout_2020..dom..BoxSlot$u20$as$u20$core..ops..drop..Drop$GT$4drop17h03488a18e530c6c8E>:
}

impl Drop for BoxSlot<'_> {
    fn drop(&mut self) {
  cf03d0:	55                   	push   rbp
  cf03d1:	48 89 e5             	mov    rbp,rsp
  cf03d4:	53                   	push   rbx
  cf03d5:	48 83 ec 58          	sub    rsp,0x58
  cf03d9:	48 8d 05 e8 88 b4 05 	lea    rax,[rip+0x5b488e8]        # 6838cc8 <_ZN3std9panicking11panic_count18GLOBAL_PANIC_COUNT17hd7fda31a6dd5a9d3E>

@delan delan merged commit bf531b7 into main Sep 23, 2024
@delan delan deleted the profiling branch September 23, 2024 08:48
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