Skip to content

Commit

Permalink
heap: Fix atomic pause UMA histograms
Browse files Browse the repository at this point in the history
The new naming scheme for GC UMA histogram used the Event prefix for all
atomic pause histograms. This was explained as: there should only be a
single atomic pause per cycle, so Event and Cycle prefixes are
equivalent in this case.
The underlying implementation assumed that there is indeed a single
atomic pause event. This assumption holds for V8 (V8.GC_MARK_COMPACTOR)
but not for Oilpan. Oilpan has a top level event for each phase of the
atomic pause (CppGC.AtomicMark, CppGC.AtomicSweep, etc.) and the overall
atomic pause is defined as the sum of these events.
When reporting atomic pause histograms using the Event prefix, we
reported each event separately (similar to incremental evnets) which
messed the actual histogram.

This CL replaces the Event prefixed Oilpan atomic pause histogram with
Cycle prefixed histograms. The histograms are kept aligned with their
gc metric counterparts.

Bug: 1056170
Change-Id: I7096334abbcc460716f89b77f08cf550d82f26ec
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3137717
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#917680}
NOKEYCHECK=True
GitOrigin-RevId: b6319aa855b86a0095d6c0c27630ffeb5d4676ba
  • Loading branch information
omerktz authored and copybara-github committed Sep 2, 2021
1 parent 4d1f8a7 commit fdc8771
Showing 1 changed file with 12 additions and 18 deletions.
30 changes: 12 additions & 18 deletions blink/renderer/bindings/core/v8/v8_metrics.cc
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,6 @@ void V8MetricsRecorder::AddMainThreadEvent(
namespace {

#if BUILDFLAG(USE_V8_OILPAN)
void ReportAtomicLatencyEvent(int64_t duration_us) {
UMA_HISTOGRAM_TIMES("V8.GC.Event.MainThread.Full.Atomic.Cpp",
base::TimeDelta::FromMicroseconds(duration_us));
}

// Helper function to convert a byte count to a KB count, capping at
// INT_MAX if the number is larger than that.
constexpr int32_t CappedSizeInKB(int64_t size_in_bytes) {
Expand Down Expand Up @@ -179,31 +174,30 @@ void V8MetricsRecorder::AddMainThreadEvent(
base::TimeDelta::FromMicroseconds(
event.main_thread_cpp.sweep_wall_clock_duration_in_us));

// Report latency metrics:
// Report atomic pause metrics:
UMA_HISTOGRAM_TIMES(
"V8.GC.Event.MainThread.Full.Atomic.Mark.Cpp",
"V8.GC.Cycle.MainThread.Full.Atomic.Mark.Cpp",
base::TimeDelta::FromMicroseconds(
event.main_thread_atomic_cpp.mark_wall_clock_duration_in_us));
UMA_HISTOGRAM_TIMES(
"V8.GC.Event.MainThread.Full.Atomic.Weak.Cpp",
"V8.GC.Cycle.MainThread.Full.Atomic.Weak.Cpp",
base::TimeDelta::FromMicroseconds(
event.main_thread_atomic_cpp.weak_wall_clock_duration_in_us));
UMA_HISTOGRAM_TIMES(
"V8.GC.Event.MainThread.Full.Atomic.Compact.Cpp",
"V8.GC.Cycle.MainThread.Full.Atomic.Compact.Cpp",
base::TimeDelta::FromMicroseconds(
event.main_thread_atomic_cpp.compact_wall_clock_duration_in_us));
UMA_HISTOGRAM_TIMES(
"V8.GC.Event.MainThread.Full.Atomic.Sweep.Cpp",
"V8.GC.Cycle.MainThread.Full.Atomic.Sweep.Cpp",
base::TimeDelta::FromMicroseconds(
event.main_thread_atomic_cpp.sweep_wall_clock_duration_in_us));
UMA_HISTOGRAM_TIMES(
"V8.GC.Cycle.MainThread.Full.Atomic.Cpp",
base::TimeDelta::FromMicroseconds(
event.main_thread_atomic_cpp.mark_wall_clock_duration_in_us +
event.main_thread_atomic_cpp.weak_wall_clock_duration_in_us +
event.main_thread_atomic_cpp.compact_wall_clock_duration_in_us +
event.main_thread_atomic_cpp.sweep_wall_clock_duration_in_us));
ReportAtomicLatencyEvent(
event.main_thread_atomic_cpp.mark_wall_clock_duration_in_us);
ReportAtomicLatencyEvent(
event.main_thread_atomic_cpp.weak_wall_clock_duration_in_us);
ReportAtomicLatencyEvent(
event.main_thread_atomic_cpp.compact_wall_clock_duration_in_us);
ReportAtomicLatencyEvent(
event.main_thread_atomic_cpp.sweep_wall_clock_duration_in_us);

// Report size metrics:
static constexpr size_t kMinSize = 1;
Expand Down

0 comments on commit fdc8771

Please sign in to comment.