From 25f65e20a5fe2886c6da04b1bad2cdff8d61e69f Mon Sep 17 00:00:00 2001 From: Kunal Sareen Date: Mon, 31 Mar 2025 01:30:39 +0000 Subject: [PATCH] Fix broken stats gathering code if no. of GCs > `MAX_PHASES` --- src/util/statistics/counter/event_counter.rs | 12 ++++--- src/util/statistics/counter/long_counter.rs | 12 ++++--- src/util/statistics/stats.rs | 35 ++++++-------------- 3 files changed, 24 insertions(+), 35 deletions(-) diff --git a/src/util/statistics/counter/event_counter.rs b/src/util/statistics/counter/event_counter.rs index 17ff07cfea..d417cf4a01 100644 --- a/src/util/statistics/counter/event_counter.rs +++ b/src/util/statistics/counter/event_counter.rs @@ -1,5 +1,5 @@ use super::*; -use crate::util::statistics::stats::{SharedStats, MAX_PHASES}; +use crate::util::statistics::stats::{SharedStats, DEFAULT_NUM_PHASES}; use std::sync::Arc; /** @@ -10,7 +10,7 @@ pub struct EventCounter { name: String, pub implicitly_start: bool, merge_phases: bool, - count: Box<[u64; MAX_PHASES]>, + count: Vec, current_count: u64, running: bool, stats: Arc, @@ -27,7 +27,7 @@ impl EventCounter { name, implicitly_start, merge_phases, - count: Box::new([0; MAX_PHASES]), + count: Vec::with_capacity(DEFAULT_NUM_PHASES), current_count: 0, running: false, stats, @@ -76,13 +76,15 @@ impl Counter for EventCounter { return; } debug_assert!(self.running); - self.count[self.stats.get_phase()] = self.current_count; + self.count.push(self.current_count); + debug_assert_eq!(self.count[self.stats.get_phase()], self.current_count); self.running = false; } fn phase_change(&mut self, old_phase: usize) { if self.running { - self.count[old_phase] = self.current_count; + self.count.push(self.current_count); + debug_assert_eq!(self.count[old_phase], self.current_count); self.current_count = 0; } } diff --git a/src/util/statistics/counter/long_counter.rs b/src/util/statistics/counter/long_counter.rs index d52b17fb3d..82348df10c 100644 --- a/src/util/statistics/counter/long_counter.rs +++ b/src/util/statistics/counter/long_counter.rs @@ -1,5 +1,5 @@ use super::*; -use crate::util::statistics::stats::{SharedStats, MAX_PHASES}; +use crate::util::statistics::stats::{SharedStats, DEFAULT_NUM_PHASES}; use std::fmt; use std::sync::Arc; @@ -7,7 +7,7 @@ pub struct LongCounter { name: String, pub implicitly_start: bool, merge_phases: bool, - count: Box<[u64; MAX_PHASES]>, // FIXME make this resizable + count: Vec, diffable: T, start_value: Option, total_count: u64, @@ -41,7 +41,8 @@ impl Counter for LongCounter { self.diffable.stop(); let current_value = self.diffable.current_value(); let delta = T::diff(¤t_value, self.start_value.as_ref().unwrap()); - self.count[self.stats.get_phase()] += delta; + self.count.push(delta); + debug_assert_eq!(self.count[self.stats.get_phase()], delta); self.total_count += delta; } @@ -49,7 +50,8 @@ impl Counter for LongCounter { if self.running { let now = self.diffable.current_value(); let delta = T::diff(&now, self.start_value.as_ref().unwrap()); - self.count[old_phase] += delta; + self.count.push(delta); + debug_assert_eq!(self.count[old_phase], delta); self.total_count += delta; self.start_value = Some(now); } @@ -139,7 +141,7 @@ impl LongCounter { name, implicitly_start, merge_phases, - count: Box::new([0; MAX_PHASES]), + count: Vec::with_capacity(DEFAULT_NUM_PHASES), diffable, start_value: None, total_count: 0, diff --git a/src/util/statistics/stats.rs b/src/util/statistics/stats.rs index 6e754b1384..e3a12a730a 100644 --- a/src/util/statistics/stats.rs +++ b/src/util/statistics/stats.rs @@ -11,9 +11,8 @@ use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::sync::Arc; use std::sync::Mutex; -// TODO: Increasing this number would cause JikesRVM die at boot time. I don't really know why. -// E.g. using 1 << 14 will cause JikesRVM segfault at boot time. -pub const MAX_PHASES: usize = 1 << 12; +/// The default number of phases for statistics. +pub const DEFAULT_NUM_PHASES: usize = 1 << 12; pub const MAX_COUNTERS: usize = 100; /// GC stats shared among counters @@ -51,10 +50,8 @@ pub struct Stats { // Initialization of libpfm4 is required before we can use `PerfEvent` types #[cfg(feature = "perf_counter")] perfmon: Perfmon, - pub shared: Arc, counters: Mutex>>>, - exceeded_phase_limit: AtomicBool, } impl Stats { @@ -99,10 +96,8 @@ impl Stats { total_time: t, #[cfg(feature = "perf_counter")] perfmon, - shared, counters: Mutex::new(counters), - exceeded_phase_limit: AtomicBool::new(false), } } @@ -157,32 +152,22 @@ impl Stats { if !self.get_gathering_stats() { return; } - if self.get_phase() < MAX_PHASES - 1 { - let counters = self.counters.lock().unwrap(); - for counter in &(*counters) { - counter.lock().unwrap().phase_change(self.get_phase()); - } - self.shared.increment_phase(); - } else if !self.exceeded_phase_limit.load(Ordering::SeqCst) { - eprintln!("Warning: number of GC phases exceeds MAX_PHASES"); - self.exceeded_phase_limit.store(true, Ordering::SeqCst); + let counters = self.counters.lock().unwrap(); + for counter in &(*counters) { + counter.lock().unwrap().phase_change(self.get_phase()); } + self.shared.increment_phase(); } pub fn end_gc(&self) { if !self.get_gathering_stats() { return; } - if self.get_phase() < MAX_PHASES - 1 { - let counters = self.counters.lock().unwrap(); - for counter in &(*counters) { - counter.lock().unwrap().phase_change(self.get_phase()); - } - self.shared.increment_phase(); - } else if !self.exceeded_phase_limit.load(Ordering::SeqCst) { - eprintln!("Warning: number of GC phases exceeds MAX_PHASES"); - self.exceeded_phase_limit.store(true, Ordering::SeqCst); + let counters = self.counters.lock().unwrap(); + for counter in &(*counters) { + counter.lock().unwrap().phase_change(self.get_phase()); } + self.shared.increment_phase(); } pub fn print_stats(&self, mmtk: &'static MMTK) {