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}
  • Loading branch information
@omerktz
omerktz authored and Chromium LUCI CQ committed Sep 2, 2021
1 parent 176eac9 commit b6319aa
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 18 deletions.
30 changes: 12 additions & 18 deletions third_party/blink/renderer/bindings/core/v8/v8_metrics.cc
View file
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
75 changes: 75 additions & 0 deletions tools/metrics/histograms/metadata/v8/histograms.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,66 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
</summary>
</histogram>

<histogram name="V8.GC.Cycle.MainThread.Full.Atomic.Compact.Cpp" units="ms"
expires_after="M100">
<owner>omerkatz@chromium.org</owner>
<owner>v8-memory-sheriffs@google.com</owner>
<summary>
Overall duration of compaction on the main thread during the atomic pause of
a garbage collection of the managed C++ heap. There is only a single atomic
pause compaction event per cycle but it may cover several sub-events.
Reported at the end of the garbage collection cycle.
</summary>
</histogram>

<histogram name="V8.GC.Cycle.MainThread.Full.Atomic.Cpp" units="ms"
expires_after="M100">
<owner>omerkatz@chromium.org</owner>
<owner>v8-memory-sheriffs@google.com</owner>
<summary>
Overall main thread duration of the atomic pause of a garbage collection of
the managed C++ heap. There is only a single atomic pause event per cycle
but it may cover several sub-events. Reported at the end of the garbage
collection cycle.
</summary>
</histogram>

<histogram name="V8.GC.Cycle.MainThread.Full.Atomic.Mark.Cpp" units="ms"
expires_after="M100">
<owner>omerkatz@chromium.org</owner>
<owner>v8-memory-sheriffs@google.com</owner>
<summary>
Overall duration of marking on the main thread during the atomic pause of a
garbage collection of the managed C++ heap. There is only a single atomic
pause marking event per cycle but it may cover several sub-events. Reported
at the end of the garbage collection cycle.
</summary>
</histogram>

<histogram name="V8.GC.Cycle.MainThread.Full.Atomic.Sweep.Cpp" units="ms"
expires_after="M100">
<owner>omerkatz@chromium.org</owner>
<owner>v8-memory-sheriffs@google.com</owner>
<summary>
Overall duration of sweeping on the main thread during the atomic pasue of a
garbage collection of the managed C++ heap. There is only a single atomic
pause sweeping event per cycle but it may cover several sub-events. Reported
at the end of the garbage collection cycle.
</summary>
</histogram>

<histogram name="V8.GC.Cycle.MainThread.Full.Atomic.Weak.Cpp" units="ms"
expires_after="M100">
<owner>omerkatz@chromium.org</owner>
<owner>v8-memory-sheriffs@google.com</owner>
<summary>
Overall duration of weakness handling on the main thread during the atomic
pause of a garbage collection of the managed C++ heap. There is only a
single atomic pause weakness handling event per cycle but it may cover
several sub-events. Reported at the end of the garbage collection cycle.
</summary>
</histogram>

<histogram name="V8.GC.Cycle.MainThread.Full.Compact.Cpp" units="ms"
expires_after="M100">
<owner>omerkatz@chromium.org</owner>
Expand Down Expand Up @@ -553,6 +613,9 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.

<histogram name="V8.GC.Event.MainThread.Full.Atomic.Compact.Cpp" units="ms"
expires_after="M100">
<obsolete>
Removed 09/2021. Renamed to V8.GC.Cycle.MainThread.Full.Atomic.Compact.Cpp.
</obsolete>
<owner>omerkatz@chromium.org</owner>
<owner>v8-memory-sheriffs@google.com</owner>
<summary>
Expand All @@ -563,6 +626,9 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.

<histogram name="V8.GC.Event.MainThread.Full.Atomic.Cpp" units="ms"
expires_after="M100">
<obsolete>
Removed 09/2021. Renamed to V8.GC.Cycle.MainThread.Full.Atomic.Cpp.
</obsolete>
<owner>omerkatz@chromium.org</owner>
<owner>v8-memory-sheriffs@google.com</owner>
<summary>
Expand All @@ -573,6 +639,9 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.

<histogram name="V8.GC.Event.MainThread.Full.Atomic.Mark.Cpp" units="ms"
expires_after="M100">
<obsolete>
Removed 09/2021. Renamed to V8.GC.Cycle.MainThread.Full.Atomic.Mark.Cpp.
</obsolete>
<owner>omerkatz@chromium.org</owner>
<owner>v8-memory-sheriffs@google.com</owner>
<summary>
Expand All @@ -583,6 +652,9 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.

<histogram name="V8.GC.Event.MainThread.Full.Atomic.Sweep.Cpp" units="ms"
expires_after="M100">
<obsolete>
Removed 09/2021. Renamed to V8.GC.Cycle.MainThread.Full.Atomic.Sweep.Cpp.
</obsolete>
<owner>omerkatz@chromium.org</owner>
<owner>v8-memory-sheriffs@google.com</owner>
<summary>
Expand All @@ -593,6 +665,9 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.

<histogram name="V8.GC.Event.MainThread.Full.Atomic.Weak.Cpp" units="ms"
expires_after="M100">
<obsolete>
Removed 09/2021. Renamed to V8.GC.Cycle.MainThread.Full.Atomic.Weak.Cpp.
</obsolete>
<owner>omerkatz@chromium.org</owner>
<owner>v8-memory-sheriffs@google.com</owner>
<summary>
Expand Down

0 comments on commit b6319aa

Please sign in to comment.