Conversation
74305c7 to
17e1d5d
Compare
15bbf97 to
327de42
Compare
0a58fef to
240e60c
Compare
|
Warning This PR has more than 500 changed lines and does not include a spec. Large features and architectural changes benefit from a spec-driven workflow. If this PR is a bug fix, refactor, or doesn't warrant a spec, feel free to ignore this message. |
Remove duplicate [workspace.lints.clippy] section introduced by rebase, add workspace lint inheritance to jolt-profiling, bump sysinfo to workspace version, and fix all lint violations.
0xAndoroid
left a comment
There was a problem hiding this comment.
7 issues found after deduplication across semantic, bug, tech-debt, and security review passes. Two earlier issues (missing workspace lints, sysinfo version mismatch) were fixed in b003fba.
| let interval = Duration::from_millis((interval_secs * 1000.0) as u64); | ||
| let mut system = System::new_all(); | ||
|
|
||
| thread::sleep(sysinfo::MINIMUM_CPU_UPDATE_INTERVAL); | ||
|
|
||
| while !stop.load(Ordering::Relaxed) { | ||
| system.refresh_all(); |
There was a problem hiding this comment.
System::new_all() initializes disk, network, component, and process lists — none of which are read. refresh_all() refreshes all of them every tick. For a monitor that only needs CPU and memory this is significant unnecessary overhead per sample.
| let interval = Duration::from_millis((interval_secs * 1000.0) as u64); | |
| let mut system = System::new_all(); | |
| thread::sleep(sysinfo::MINIMUM_CPU_UPDATE_INTERVAL); | |
| while !stop.load(Ordering::Relaxed) { | |
| system.refresh_all(); | |
| let interval = Duration::from_millis((interval_secs * 1000.0) as u64); | |
| let mut system = System::new(); | |
| thread::sleep(sysinfo::MINIMUM_CPU_UPDATE_INTERVAL); | |
| while !stop.load(Ordering::Relaxed) { | |
| system.refresh_cpu_all(); |
| /// | ||
| /// Uses GiB for values >= 1.0, otherwise MiB. | ||
| pub fn format_memory_size(gib: f64) -> String { | ||
| if gib >= 1.0 { |
There was a problem hiding this comment.
Memory deltas from memory.rs can be negative (memory freed during a span). A delta of -2.0 GiB fails the >= 1.0 check, falls into the MiB branch, and formats as "-2048.00 MiB" instead of "-2.00 GiB".
| if gib >= 1.0 { | |
| if gib.abs() >= 1.0 { |
| let mut opts = Options::default(); | ||
| opts.color_diffusion = true; | ||
| opts.count_name = String::from("MiB"); | ||
| opts.factor = 1.0 / BYTES_PER_GIB * 1024.0; |
There was a problem hiding this comment.
The intent is bytes→MiB (matching count_name = "MiB"), but the expression takes a detour through GiB. BYTES_PER_MIB is already defined in units.
| opts.factor = 1.0 / BYTES_PER_GIB * 1024.0; | |
| opts.factor = 1.0 / BYTES_PER_MIB; |
Also update the import on line 8 to bring in BYTES_PER_MIB instead of (or in addition to) BYTES_PER_GIB.
| let mut memory_usage_map = MEMORY_USAGE_MAP.lock().unwrap(); | ||
| let memory_gib_start = memory_usage_map | ||
| .remove(label) | ||
| .unwrap_or_else(|| panic!("no open memory span: {label}")); | ||
|
|
||
| let delta = memory_gib_end - memory_gib_start; | ||
| let mut memory_delta_map = MEMORY_DELTA_MAP.lock().unwrap(); | ||
| assert_eq!(memory_delta_map.insert(label, delta), None); |
There was a problem hiding this comment.
MEMORY_USAGE_MAP is held while acquiring MEMORY_DELTA_MAP. If the assert_eq! on line 67 panics, the first mutex is poisoned and every subsequent call to start_memory_tracing_span/end_memory_tracing_span panics with PoisonError. Drop the first lock before acquiring the second:
| let mut memory_usage_map = MEMORY_USAGE_MAP.lock().unwrap(); | |
| let memory_gib_start = memory_usage_map | |
| .remove(label) | |
| .unwrap_or_else(|| panic!("no open memory span: {label}")); | |
| let delta = memory_gib_end - memory_gib_start; | |
| let mut memory_delta_map = MEMORY_DELTA_MAP.lock().unwrap(); | |
| assert_eq!(memory_delta_map.insert(label, delta), None); | |
| let memory_gib_start = { | |
| let mut map = MEMORY_USAGE_MAP.lock().unwrap(); | |
| map.remove(label) | |
| .unwrap_or_else(|| panic!("no open memory span: {label}")) | |
| }; | |
| let delta = memory_gib_end - memory_gib_start; | |
| let mut memory_delta_map = MEMORY_DELTA_MAP.lock().unwrap(); | |
| assert_eq!(memory_delta_map.insert(label, delta), None); |
| let handle = thread::Builder::new() | ||
| .name("metrics-monitor".to_string()) | ||
| .spawn(move || { | ||
| let interval = Duration::from_millis((interval_secs * 1000.0) as u64); |
There was a problem hiding this comment.
(negative_f64 * 1000.0) as u64 saturates to 0, and NaN also casts to 0. This creates a Duration::ZERO sleep, turning the loop into a CPU spin. Clamp to a sane minimum.
| let interval = Duration::from_millis((interval_secs * 1000.0) as u64); | |
| let interval = Duration::from_millis(((interval_secs * 1000.0) as u64).max(50)); |
| #[cfg(feature = "allocative")] | ||
| pub mod flamegraph; |
There was a problem hiding this comment.
Every other module's public functions are re-exported at the crate root (memory::*, monitor::MetricsMonitor, setup::*). flamegraph is the exception — callers must use jolt_profiling::flamegraph::write_flamegraph_svg. Consider adding:
#[cfg(feature = "allocative")]
pub use flamegraph::{print_data_structure_heap_usage, write_flamegraph_svg};| while !stop.load(Ordering::Relaxed) { | ||
| system.refresh_all(); | ||
|
|
||
| let memory_gib = memory_stats() | ||
| .map(|s| s.physical_mem as f64 / BYTES_PER_GIB) | ||
| .unwrap_or(0.0); | ||
| let cpu_percent = system.global_cpu_usage(); | ||
| let cores_active_avg = cpu_percent / 100.0 * (system.cpus().len() as f32); | ||
| let active_cores = system | ||
| .cpus() | ||
| .iter() | ||
| .filter(|cpu| cpu.cpu_usage() > 0.1) | ||
| .count(); | ||
|
|
||
| #[cfg(target_os = "linux")] | ||
| let active_threads = std::fs::read_dir("/proc/self/task") | ||
| .map(|entries| entries.count()) | ||
| .unwrap_or(0); | ||
|
|
||
| #[cfg(not(target_os = "linux"))] | ||
| let active_threads = 0_usize; | ||
|
|
||
| tracing::debug!( | ||
| counters.memory_gib = memory_gib, | ||
| counters.cpu_percent = cpu_percent, | ||
| counters.cores_active_avg = cores_active_avg, | ||
| counters.cores_active = active_cores, | ||
| counters.thread_count = active_threads, | ||
| ); | ||
|
|
||
| thread::sleep(interval); | ||
| } | ||
|
|
||
| tracing::info!("MetricsMonitor stopping"); | ||
| }) | ||
| .expect("Failed to spawn metrics monitor thread"); | ||
|
|
||
| MetricsMonitor { | ||
| handle: Some(handle), | ||
| stop_flag, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl Drop for MetricsMonitor { | ||
| fn drop(&mut self) { | ||
| self.stop_flag.store(true, Ordering::Relaxed); |
There was a problem hiding this comment.
Ordering::Relaxed on both the load (line 49) and store (line 95) is technically safe — join() provides the final synchronization — but on weakly-ordered architectures the thread may run extra iterations after Drop signals stop. Standard pattern for stop flags is Release (store) / Acquire (load). Minor, but it's a one-word change.
Benchmark comparison (crates) |

Summary
Changes
Testing
cargo clippyandcargo fmtpassSecurity Considerations
Breaking Changes
None