From 376f04756e42cd08aaab42d9bfd2e0e3d9c2b660 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Stig=20Rohde=20D=C3=B8ssing?= <stigdoessing@gmail.com>
Date: Tue, 21 May 2024 18:56:50 +0200
Subject: [PATCH] Adjust object sampler span handling

The span stored in each sample is not the calculated span, it's just the
object's byte size (`allocated`). That means as soon as any object falls
out of the queue, the spans in the queue no longer sum to cover the
allocation timeline. This causes all future samples to be added to be
unduly prioritized for adding to the queue, because they are given an
artificially high span. In effect, future samples are weighted as if they
cover both the interval between themselves and the older neighbor sample,
plus all "missing spans" from nodes that have been discarded since the
program started.

Changed object samples to store the calculated span rather than the bytes
allocated for the sampled object.

When a sample is removed from the queue because a sample with a larger
span is being added, the span of the removed node is not handed to the
younger neighbor, this only happens when a sample is removed due to GC.
This means that the span will be given to the next sample added to the
queue. When the sample being removed is the youngest sample, this is fine,
but when it's a sample that has a younger neighbor, the span should
probably be given to that neighbor rather than the newcomer. Handing it to
the newcomer gives the new sample a high weight it doesn't deserve. It ends
up covering not just the span to the older neighbor, but also the span of
the removed node, which is not what we want.

When replacing a sample in the queue, give the span of the removed sample
to the younger neighbor. If there is no such neighbor, because the youngest
sample is being replaced, give the span to the node being added instead,
as that will become the new youngest sample.
---
 .../leakprofiler/sampling/objectSampler.cpp   | 28 +++++++++++++++----
 .../leakprofiler/sampling/objectSampler.hpp   |  1 +
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.cpp b/src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.cpp
index c80d93619a566..e4f822a407fde 100644
--- a/src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.cpp
+++ b/src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.cpp
@@ -258,12 +258,25 @@ void ObjectSampler::add(HeapWord* obj, size_t allocated, traceid thread_id, bool
       // quick reject, will not fit
       return;
     }
-    sample = _list->reuse(_priority_queue->pop());
+    ObjectSample* popped = _priority_queue->pop();
+    size_t popped_span = popped->span();
+    ObjectSample* previous = popped->prev();
+    sample = _list->reuse(popped);
+    assert(sample != nullptr, "invariant");
+    if (previous != nullptr) {
+      push_span(previous, popped_span);
+      sample->set_span(span);
+    } else {
+      // The removed sample was the youngest sample in the list, which means the new sample is now the youngest
+      // sample. It should cover the spans of both.
+      sample->set_span(span + popped_span);
+    }
   } else {
     sample = _list->get();
+    assert(sample != nullptr, "invariant");
+    sample->set_span(span);
   }
 
-  assert(sample != nullptr, "invariant");
   signal_unresolved_entry();
   sample->set_thread_id(thread_id);
   if (virtual_thread) {
@@ -278,7 +291,6 @@ void ObjectSampler::add(HeapWord* obj, size_t allocated, traceid thread_id, bool
     sample->set_stack_trace_hash(stacktrace_hash);
   }
 
-  sample->set_span(allocated);
   sample->set_object(cast_to_oop(obj));
   sample->set_allocated(allocated);
   sample->set_allocation_time(JfrTicks::now());
@@ -305,14 +317,18 @@ void ObjectSampler::remove_dead(ObjectSample* sample) {
   ObjectSample* const previous = sample->prev();
   // push span onto previous
   if (previous != nullptr) {
-    _priority_queue->remove(previous);
-    previous->add_span(sample->span());
-    _priority_queue->push(previous);
+    push_span(previous, sample->span());
   }
   _priority_queue->remove(sample);
   _list->release(sample);
 }
 
+void ObjectSampler::push_span(ObjectSample* sample, size_t span) {
+    _priority_queue->remove(sample);
+    sample->add_span(span);
+    _priority_queue->push(sample);
+}
+
 ObjectSample* ObjectSampler::last() const {
   return _list->last();
 }
diff --git a/src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.hpp b/src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.hpp
index e6bb16506c982..25b5a67dd2a85 100644
--- a/src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.hpp
+++ b/src/hotspot/share/jfr/leakprofiler/sampling/objectSampler.hpp
@@ -64,6 +64,7 @@ class ObjectSampler : public CHeapObj<mtTracing> {
   void add(HeapWord* object, size_t size, traceid thread_id, bool virtual_thread, const JfrBlobHandle& bh, JavaThread* thread);
   void scavenge();
   void remove_dead(ObjectSample* sample);
+  void push_span(ObjectSample* sample, size_t span);
 
   const ObjectSample* item_at(int index) const;
   ObjectSample* item_at(int index);