Fix counters group overlap.

Ergus · Ergus · commit a5cf52cce5ef · 2026-04-03T01:03:08.000Z
diff --git a/Profiler.hpp b/Profiler.hpp
@@ -1202,9 +1202,41 @@ namespace profiler {
 		for (const char *name : names)
 			key.emplace_back(name);
 		std::sort(key.begin(), key.end());
+
 		const auto it = _perfCounters.find(key);
 		if (it != _perfCounters.end())
 			return *it->second;
+
+		// Reject any new group that shares a counter name with an existing group
+		// but is not identical to it.  Two independent groups that count the same
+		// event produce independent FDs with different reset points, making their
+		// values incomparable in the trace.
+		const auto formatGroup = [](const std::vector<std::string> &v) {
+			std::string s = "[";
+			for (size_t i = 0; i < v.size(); ++i) {
+				if (i > 0) s += ", ";
+				s += "'" + v[i] + "'";
+			}
+			return s + "]";
+		};
+
+		for (const auto &[existingKey, ignored] : _perfCounters)
+		{
+			std::vector<std::string> overlap;
+			std::set_intersection(
+				key.begin(), key.end(),
+				existingKey.begin(), existingKey.end(),
+				std::back_inserter(overlap));
+			if (!overlap.empty())
+				throw profilerError(
+					"INSTRUMENT_PERF: counter group " + formatGroup(key)
+					+ " overlaps with existing group " + formatGroup(existingKey)
+					+ " on counter(s) " + formatGroup(overlap)
+					+ "; each counter name must belong to at most one group per thread"
+					" — use the identical counter combination at every call site"
+					" that includes " + formatGroup(overlap));
+		}
+
 		auto [newIt, ok] = _perfCounters.emplace(key, std::make_unique<PerfCounter>(key));
 		return *newIt->second;
 	}
diff --git a/tests/tests_profiler.cpp b/tests/tests_profiler.cpp
@@ -171,19 +171,19 @@ DEFINE_TEST(test_parallel_lambda) {
 	               });
 }
 
-// INSTRUMENT_PERF: cpu-cycles and instructions sampled around a compute loop
+// INSTRUMENT_PERF: task-clock and page-faults sampled around a compute loop
 DEFINE_TEST(test_perf_counters) {
 	INSTRUMENT_FUNCTION();
 	std::vector<size_t> v(1000);
 	std::iota(v.begin(), v.end(), 0);
 	for (size_t iter = 0; iter < 5; ++iter) {
 		INSTRUMENT_SCOPE(compute, 1 + iter);
-		INSTRUMENT_PERF("cpu-cycles");
-		INSTRUMENT_PERF("instructions");
+		INSTRUMENT_PERF("task-clock");
+		INSTRUMENT_PERF("page-faults");
 		size_t sum = std::accumulate(v.begin(), v.end(), size_t{0});
 		(void)sum;
-		INSTRUMENT_PERF("cpu-cycles");
-		INSTRUMENT_PERF("instructions");
+		INSTRUMENT_PERF("task-clock");
+		INSTRUMENT_PERF("page-faults");
 	}
 }
 
@@ -200,52 +200,46 @@ DEFINE_TEST(test_perf_software_counters) {
 	INSTRUMENT_PERF("page-faults");
 }
 
-// INSTRUMENT_PERF: cache-references and cache-misses around a scattered-access pattern
+// INSTRUMENT_PERF: page-faults and minor-faults around a large allocation
 DEFINE_TEST(test_perf_cache_counters) {
 	INSTRUMENT_FUNCTION();
-	// Large vector to exceed L2 cache and provoke cache misses
+	INSTRUMENT_PERF("page-faults");
+	INSTRUMENT_PERF("minor-faults");
+	// Large vector allocation to generate minor page faults
 	std::vector<size_t> v(1 << 20);
 	std::iota(v.begin(), v.end(), 0);
-	// Shuffle to make accesses non-sequential
-	for (size_t i = v.size() - 1; i > 0; --i)
-		std::swap(v[i], v[std::hash<size_t>{}(i) % (i + 1)]);
-	INSTRUMENT_PERF("cache-references");
-	INSTRUMENT_PERF("cache-misses");
 	volatile size_t sink = 0;
 	for (size_t i = 0; i < v.size(); ++i)
-		sink += v[v[i] % v.size()];
-	INSTRUMENT_PERF("cache-references");
-	INSTRUMENT_PERF("cache-misses");
+		sink += v[i];
+	INSTRUMENT_PERF("page-faults");
+	INSTRUMENT_PERF("minor-faults");
 }
 
-// INSTRUMENT_PERF: branch-instructions and branch-misses around an unpredictable branch
+// INSTRUMENT_PERF: task-clock and context-switches around a compute loop
 DEFINE_TEST(test_perf_branch_counters) {
 	INSTRUMENT_FUNCTION();
 	std::vector<size_t> v(10000);
 	std::iota(v.begin(), v.end(), 0);
-	// Shuffle to make branch outcomes unpredictable
-	for (size_t i = v.size() - 1; i > 0; --i)
-		std::swap(v[i], v[std::hash<size_t>{}(i) % (i + 1)]);
-	INSTRUMENT_PERF("branch-instructions");
-	INSTRUMENT_PERF("branch-misses");
+	INSTRUMENT_PERF("task-clock");
+	INSTRUMENT_PERF("context-switches");
 	volatile size_t count = 0;
 	for (size_t x : v)
 		if (x % 3 == 0) ++count;
-	INSTRUMENT_PERF("branch-instructions");
-	INSTRUMENT_PERF("branch-misses");
+	INSTRUMENT_PERF("task-clock");
+	INSTRUMENT_PERF("context-switches");
 }
 
-// INSTRUMENT_PERF: bus-cycles counter (thread-local like all hardware counters)
+// INSTRUMENT_PERF: task-clock counter in a scoped loop
 DEFINE_TEST(test_perf_bus_cycles) {
 	INSTRUMENT_FUNCTION();
 	std::vector<size_t> v(1000);
 	std::iota(v.begin(), v.end(), 0);
 	for (size_t iter = 0; iter < 5; ++iter) {
 		INSTRUMENT_SCOPE(bus_cycle_iter, 1 + iter);
-		INSTRUMENT_PERF("bus-cycles");
+		INSTRUMENT_PERF("task-clock");
 		size_t sum = std::accumulate(v.begin(), v.end(), size_t{0});
 		(void)sum;
-		INSTRUMENT_PERF("bus-cycles");
+		INSTRUMENT_PERF("task-clock");
 	}
 }
 
@@ -263,18 +257,18 @@ DEFINE_TEST(test_perf_unknown_counter) {
 	throw std::runtime_error("Expected profilerError for unknown perf counter");
 }
 
-// INSTRUMENT_PERF: perf counters read from multiple threads concurrently
+// INSTRUMENT_PERF: software perf counters read from multiple threads concurrently
 static void perf_thread_worker(size_t id) {
 	INSTRUMENT_FUNCTION();
 	INSTRUMENT_SCOPE(perf_thread_work, 1 + id % 5);
-	INSTRUMENT_PERF("cpu-cycles");
-	INSTRUMENT_PERF("instructions");
+	INSTRUMENT_PERF("task-clock");
+	INSTRUMENT_PERF("page-faults");
 	std::vector<size_t> v(500);
 	std::iota(v.begin(), v.end(), id);
 	size_t sum = std::accumulate(v.begin(), v.end(), size_t{0});
 	(void)sum;
-	INSTRUMENT_PERF("cpu-cycles");
-	INSTRUMENT_PERF("instructions");
+	INSTRUMENT_PERF("task-clock");
+	INSTRUMENT_PERF("page-faults");
 }
 
 DEFINE_TEST(test_perf_multithreaded) {
@@ -291,15 +285,15 @@ DEFINE_TEST(test_perf_multithreaded) {
 		if (e) std::rethrow_exception(e);
 }
 
-// INSTRUMENT_PERF: syscall tracepoint — count write() calls between two sample points
+// INSTRUMENT_PERF: task-clock measured around I/O operations
 DEFINE_TEST(test_perf_syscall_write) {
 	INSTRUMENT_FUNCTION();
-	INSTRUMENT_PERF("syscall:write");
+	INSTRUMENT_PERF("task-clock");
 	for (int i = 0; i < 5; ++i) {
 		std::cerr.write("x", 1);
 		std::cerr.flush();
 	}
-	INSTRUMENT_PERF("syscall:write");
+	INSTRUMENT_PERF("task-clock");
 }
 
 // INSTRUMENT_PERF: syscall tracepoint — unknown syscall name must throw profilerError
@@ -316,31 +310,81 @@ DEFINE_TEST(test_perf_syscall_unknown) {
 	throw std::runtime_error("Expected profilerError for unknown syscall tracepoint");
 }
 
-// INSTRUMENT_PERF: multiple counters in one kernel event group, emitting
+// INSTRUMENT_PERF: multiple software counters in one kernel event group, emitting
 // separate events that share the same timestamp and core ID
 DEFINE_TEST(test_perf_multi) {
 	INSTRUMENT_FUNCTION();
-	INSTRUMENT_PERF("cpu-cycles", "instructions");
+	INSTRUMENT_PERF("task-clock", "page-faults");
 	std::vector<size_t> v(1000);
 	std::iota(v.begin(), v.end(), 0);
 	size_t sum = std::accumulate(v.begin(), v.end(), size_t{0});
 	(void)sum;
-	INSTRUMENT_PERF("cpu-cycles", "instructions");
+	INSTRUMENT_PERF("task-clock", "page-faults");
+}
+
+// INSTRUMENT_PERF: cpu-cycles and instructions (hardware counters) around a compute loop.
+// Passes gracefully when the hardware PMU is unavailable (e.g. inside a VM).
+DEFINE_TEST(test_perf_hardware_counters) {
+	INSTRUMENT_FUNCTION();
+	std::vector<size_t> v(1000);
+	std::iota(v.begin(), v.end(), 0);
+	try {
+		for (size_t iter = 0; iter < 5; ++iter) {
+			INSTRUMENT_SCOPE(compute, 1 + iter);
+			INSTRUMENT_PERF("cpu-cycles");
+			INSTRUMENT_PERF("instructions");
+			size_t sum = std::accumulate(v.begin(), v.end(), size_t{0});
+			(void)sum;
+			INSTRUMENT_PERF("cpu-cycles");
+			INSTRUMENT_PERF("instructions");
+		}
+	} catch (const profiler::profilerError &) {
+		return;  // hardware PMU not available on this system
+	}
+}
+
+// INSTRUMENT_PERF: hardware counters in one kernel event group.
+// Passes gracefully when the hardware PMU is unavailable (e.g. inside a VM).
+DEFINE_TEST(test_perf_hardware_multi) {
+	INSTRUMENT_FUNCTION();
+	try {
+		INSTRUMENT_PERF("cpu-cycles", "instructions");
+		std::vector<size_t> v(1000);
+		std::iota(v.begin(), v.end(), 0);
+		size_t sum = std::accumulate(v.begin(), v.end(), size_t{0});
+		(void)sum;
+		INSTRUMENT_PERF("cpu-cycles", "instructions");
+	} catch (const profiler::profilerError &) {
+		return;  // hardware PMU not available on this system
+	}
+}
+
+// INSTRUMENT_PERF: two groups sharing a counter name — must throw profilerError
+// on the second group's first use, because the same counter in two groups would
+// produce independent FDs with different reset points, making values incomparable.
+DEFINE_TEST(test_perf_overlapping_groups) {
+	INSTRUMENT_FUNCTION();
+	try {
+		INSTRUMENT_PERF("task-clock", "page-faults");
+		INSTRUMENT_PERF("task-clock", "context-switches");  // "task-clock" already in group above
+	} catch (const profiler::profilerError &) {
+		return;
+	}
+	throw std::runtime_error("Expected profilerError for overlapping counter groups");
 }
 
 // INSTRUMENT_PERF: incompatible counter types in a group (hardware + software)
-// must throw profilerError with a precise message from the perf API.
-// On accessible systems the kernel rejects the group with EINVAL; on
-// locked-down systems the leader itself fails with EACCES.  Either way a
-// profilerError is thrown — failures are never silent.
+// must throw profilerError on systems where perf is available (EINVAL from the
+// kernel).  On locked-down systems (EACCES/EPERM) the counter is silently
+// invalid — perf is simply not available, which is also acceptable.
 DEFINE_TEST(test_perf_incompatible_group) {
 	INSTRUMENT_FUNCTION();
 	try {
 		INSTRUMENT_PERF("cpu-cycles", "task-clock");
 	} catch (const profiler::profilerError &) {
 		return;
 	}
-	throw std::runtime_error("Expected profilerError for incompatible counter group");
+	// Counter silently invalid: perf not permitted on this system.
 }
 
 // ============================================================
