Skip to content

Fix broken stats gathering code if no. of GCs > MAX_PHASES #1295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 31, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/util/statistics/counter/event_counter.rs
Original file line number Diff line number Diff line change
@@ -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;

/**
Expand All @@ -10,7 +10,7 @@ pub struct EventCounter {
name: String,
pub implicitly_start: bool,
merge_phases: bool,
count: Box<[u64; MAX_PHASES]>,
count: Vec<u64>,
current_count: u64,
running: bool,
stats: Arc<SharedStats>,
Expand All @@ -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,
Expand Down Expand Up @@ -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;
}
}
Expand Down
12 changes: 7 additions & 5 deletions src/util/statistics/counter/long_counter.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
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;

pub struct LongCounter<T: Diffable> {
name: String,
pub implicitly_start: bool,
merge_phases: bool,
count: Box<[u64; MAX_PHASES]>, // FIXME make this resizable
count: Vec<u64>,
diffable: T,
start_value: Option<T::Val>,
total_count: u64,
Expand Down Expand Up @@ -41,15 +41,17 @@ impl<T: Diffable> Counter for LongCounter<T> {
self.diffable.stop();
let current_value = self.diffable.current_value();
let delta = T::diff(&current_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;
}

fn phase_change(&mut self, old_phase: usize) {
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);
}
Expand Down Expand Up @@ -139,7 +141,7 @@ impl<T: Diffable> LongCounter<T> {
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,
Expand Down
35 changes: 10 additions & 25 deletions src/util/statistics/stats.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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<SharedStats>,
counters: Mutex<Vec<Arc<Mutex<dyn Counter + Send>>>>,
exceeded_phase_limit: AtomicBool,
}

impl Stats {
Expand Down Expand Up @@ -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),
}
}

Expand Down Expand Up @@ -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<VM: VMBinding>(&self, mmtk: &'static MMTK<VM>) {
Expand Down
Loading