From 87aa3019ed23f4aec428be007bc49bdca6d8dc04 Mon Sep 17 00:00:00 2001 From: Francisco Javier Honduvilla Coto Date: Mon, 11 Nov 2024 15:31:03 +0000 Subject: [PATCH] Use enums rather than basic types for compact unwind row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Or, use the type system :) Test Plan ========= CI + some simple benchmarks with `--release` to ensure that performance did not degrade ``` $ hyperfine "target/lightswitch-new --show-info /proc /2154884/root/nix/store/dwzq3jgydz8wwa7xh86f8yfxg4isnjkj-clickhouse-24.3.2.23/bin/clickhouse" "target/light switch-main --show-info /proc/2154884/root/nix/store/dwzq3jgydz8wwa7xh86f8yfxg4isnjkj-clickhouse-24.3.2.23/ bin/clickhouse" Benchmark 1: target/lightswitch-new --show-info /proc/2154884/root/nix/store/dwzq3jgydz8wwa7xh86f8yfxg4isnjkj-clickhouse-24.3.2.23/bin/clickhouse Time (mean ± σ): 805.8 ms ± 9.5 ms [User: 778.6 ms, System: 24.9 ms] Range (min … max): 792.8 ms … 817.7 ms 10 runs Benchmark 2: target/lightswitch-main --show-info /proc/2154884/root/nix/store/dwzq3jgydz8wwa7xh86f8yfxg4isnjkj-clickhouse-24.3.2.23/bin/clickhouse Time (mean ± σ): 804.6 ms ± 9.9 ms [User: 777.9 ms, System: 24.8 ms] Range (min … max): 792.7 ms … 825.0 ms 10 runs Summary target/lightswitch-main --show-info /proc/2154884/root/nix/store/dwzq3jgydz8wwa7xh86f8yfxg4isnjkj-clickhouse-24.3.2.23/bin/clickhouse ran 1.00 ± 0.02 times faster than target/lightswitch-new --show-info /proc/2154884/root/nix/store/dwzq3jgydz8wwa7xh86f8yfxg4isnjkj-clickhouse-24.3.2.23/bin/clickhouse ``` --- src/bpf/profiler_bindings.rs | 4 ++-- src/cli/main.rs | 2 +- src/unwind_info/convert.rs | 20 ++++++++++---------- src/unwind_info/optimize.rs | 4 ++-- src/unwind_info/types.rs | 14 +++++++++----- 5 files changed, 24 insertions(+), 20 deletions(-) diff --git a/src/bpf/profiler_bindings.rs b/src/bpf/profiler_bindings.rs index 29f5c49..45075c6 100644 --- a/src/bpf/profiler_bindings.rs +++ b/src/bpf/profiler_bindings.rs @@ -82,8 +82,8 @@ impl From<&CompactUnwindRow> for stack_unwind_row_t { // https://github.com/rust-lang/rust-bindgen/issues/923#issuecomment-2385554573 pc_low: (row.pc & LOW_PC_MASK as u64) as u16, cfa_offset: row.cfa_offset, - cfa_type: row.cfa_type, - rbp_type: row.rbp_type, + cfa_type: row.cfa_type as u8, + rbp_type: row.rbp_type as u8, rbp_offset: row.rbp_offset, } } diff --git a/src/cli/main.rs b/src/cli/main.rs index 4ea114c..0b6576b 100644 --- a/src/cli/main.rs +++ b/src/cli/main.rs @@ -226,7 +226,7 @@ fn show_unwind_info(path: &str) { let rbp_offset = compact_row.rbp_offset; println!( "pc: {:x} cfa_type: {:<2} rbp_type: {:<2} cfa_offset: {:<4} rbp_offset: {:<4}", - pc, cfa_type, rbp_type, cfa_offset, rbp_offset + pc, cfa_type as u8, rbp_type as u8, cfa_offset, rbp_offset ); } } diff --git a/src/unwind_info/convert.rs b/src/unwind_info/convert.rs index a7eedb2..0f4b14a 100644 --- a/src/unwind_info/convert.rs +++ b/src/unwind_info/convert.rs @@ -138,11 +138,11 @@ impl<'a> CompactUnwindInfoBuilder<'a> { match row.cfa() { CfaRule::RegisterAndOffset { register, offset } => { if register == &RBP_X86 { - compact_row.cfa_type = CfaType::FramePointerOffset as u8; + compact_row.cfa_type = CfaType::FramePointerOffset; } else if register == &RSP_X86 { - compact_row.cfa_type = CfaType::StackPointerOffset as u8; + compact_row.cfa_type = CfaType::StackPointerOffset; } else { - compact_row.cfa_type = CfaType::UnsupportedRegisterOffset as u8; + compact_row.cfa_type = CfaType::UnsupportedRegisterOffset; } match u16::try_from(*offset) { @@ -150,7 +150,7 @@ impl<'a> CompactUnwindInfoBuilder<'a> { compact_row.cfa_offset = off; } Err(_) => { - compact_row.cfa_type = CfaType::OffsetDidNotFit as u8; + compact_row.cfa_type = CfaType::OffsetDidNotFit; } } } @@ -167,29 +167,29 @@ impl<'a> CompactUnwindInfoBuilder<'a> { compact_row.cfa_offset = PltType::Plt2 as u16; } - compact_row.cfa_type = CfaType::Expression as u8; + compact_row.cfa_type = CfaType::Expression; } }; match row.register(RBP_X86) { gimli::RegisterRule::Undefined => {} gimli::RegisterRule::Offset(offset) => { - compact_row.rbp_type = RbpType::CfaOffset as u8; + compact_row.rbp_type = RbpType::CfaOffset; match i16::try_from(offset) { Ok(off) => { compact_row.rbp_offset = off; } Err(_) => { - compact_row.rbp_type = RbpType::OffsetDidNotFit as u8; + compact_row.rbp_type = RbpType::OffsetDidNotFit; } } } gimli::RegisterRule::Register(_reg) => { - compact_row.rbp_type = RbpType::Register as u8; + compact_row.rbp_type = RbpType::Register; } gimli::RegisterRule::Expression(_) => { - compact_row.rbp_type = RbpType::Expression as u8; + compact_row.rbp_type = RbpType::Expression; } _ => { // print!(", rbp unsupported {:?}", rbp); @@ -199,7 +199,7 @@ impl<'a> CompactUnwindInfoBuilder<'a> { if row.register(fde.cie().return_address_register()) == gimli::RegisterRule::Undefined { - compact_row.rbp_type = RbpType::UndefinedReturnAddress as u8; + compact_row.rbp_type = RbpType::UndefinedReturnAddress; } } _ => continue, diff --git a/src/unwind_info/optimize.rs b/src/unwind_info/optimize.rs index d724ae4..7ffa62a 100644 --- a/src/unwind_info/optimize.rs +++ b/src/unwind_info/optimize.rs @@ -16,7 +16,7 @@ pub fn remove_unnecesary_markers(unwind_info: &mut Vec) { if let Some(last_row_unwrapped) = last_row { let previous_is_redundant_marker = (last_row_unwrapped.cfa_type - == CfaType::EndFdeMarker as u8) + == CfaType::EndFdeMarker) && last_row_unwrapped.pc == row.pc; if previous_is_redundant_marker { new_i -= 1; @@ -26,7 +26,7 @@ pub fn remove_unnecesary_markers(unwind_info: &mut Vec) { let mut current_is_redundant_marker = false; if let Some(last_row_unwrapped) = last_row { current_is_redundant_marker = - (row.cfa_type == CfaType::EndFdeMarker as u8) && last_row_unwrapped.pc == row.pc; + (row.cfa_type == CfaType::EndFdeMarker) && last_row_unwrapped.pc == row.pc; } if !current_is_redundant_marker { diff --git a/src/unwind_info/types.rs b/src/unwind_info/types.rs index 50f4aac..b39807d 100644 --- a/src/unwind_info/types.rs +++ b/src/unwind_info/types.rs @@ -1,8 +1,10 @@ use lazy_static::lazy_static; #[repr(u8)] +#[derive(Debug, Default, Copy, Clone, PartialEq)] pub enum CfaType { - // Unknown = 0, + #[default] + Unknown = 0, FramePointerOffset = 1, StackPointerOffset = 2, Expression = 3, @@ -12,8 +14,10 @@ pub enum CfaType { } #[repr(u8)] +#[derive(Debug, Default, Copy, Clone, PartialEq)] pub enum RbpType { - // Unknown = 0, + #[default] + Unknown = 0, CfaOffset = 1, Register = 2, Expression = 3, @@ -31,8 +35,8 @@ pub enum PltType { #[derive(Debug, Default, Copy, Clone, PartialEq)] pub struct CompactUnwindRow { pub pc: u64, - pub cfa_type: u8, - pub rbp_type: u8, + pub cfa_type: CfaType, + pub rbp_type: RbpType, pub cfa_offset: u16, pub rbp_offset: i16, } @@ -41,7 +45,7 @@ impl CompactUnwindRow { pub fn end_of_function_marker(last_addr: u64) -> CompactUnwindRow { CompactUnwindRow { pc: last_addr, - cfa_type: CfaType::EndFdeMarker as u8, + cfa_type: CfaType::EndFdeMarker, ..Default::default() } }