Skip to content

Commit

Permalink
Use enums rather than basic types for compact unwind row
Browse files Browse the repository at this point in the history 
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
```
  • Loading branch information
@javierhonduco
javierhonduco committed Nov 11, 2024
1 parent 78fa49b commit 87aa301
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 20 deletions.
4 changes: 2 additions & 2 deletions src/bpf/profiler_bindings.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/cli/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
);
}
}
Expand Down
20 changes: 10 additions & 10 deletions src/unwind_info/convert.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,19 +138,19 @@ 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) {
Ok(off) => {
compact_row.cfa_offset = off;
}
Err(_) => {
compact_row.cfa_type = CfaType::OffsetDidNotFit as u8;
compact_row.cfa_type = CfaType::OffsetDidNotFit;
}
}
}
Expand All @@ -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);
Expand All @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions src/unwind_info/optimize.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ pub fn remove_unnecesary_markers(unwind_info: &mut Vec<CompactUnwindRow>) {

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;
Expand All @@ -26,7 +26,7 @@ pub fn remove_unnecesary_markers(unwind_info: &mut Vec<CompactUnwindRow>) {
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 {
Expand Down
14 changes: 9 additions & 5 deletions src/unwind_info/types.rs
View file
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -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,
Expand All @@ -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,
}
Expand All @@ -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()
}
}
Expand Down

0 comments on commit 87aa301

Please sign in to comment.