Skip to content

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