From 285e0c098f946c6f7a3a62a5f75934663929207a Mon Sep 17 00:00:00 2001
From: Marc Dumais <marc.dumais@ericsson.com>
Date: Wed, 28 May 2025 14:33:50 -0400
Subject: [PATCH] [SnapshotHandler] Small improvements

I noticed a few things by inspecting the code of SnapshotHandler, that
I am attempting to address in this commit:

In publish():
- a call is made to super.publish(), which would call FileHandler's
publish(). This seems unnecessary - if the file handler configuration
is present, it could result in double-publishing of log records - in
the snapshot log and in the file log, or more likely do nothing.
- instead, call the overridden isLoggable() to check whether the log
records should be logged, and if so, "publish" to the in-memory queue.

In the local isLoggable():
- remove the redundant null check, which is performed already in
super.isLoggable()
- remove the seemingly strange log level check that filters-out
anything higher than "FINE".

In addToSnapshot():
- update the in-memory "stack-tracking" structure: when the last entry
for a given pid is removed, remove the corresponding list. Similarly,
if that list was the last entry for the pid-level parent structure,
remove that one as well. The lists will be recreated if needed later
(using the existing computeIfAbsent()). This change might result in
these lists being allocated/freed multiple times, but should prevent
the case where a program, which creates many threads, ends-up with
very many of these empty lists staying allocated indefinitely, each
consuming a little heap memory.
- Make the draining of the in-memory queue to file atomic by having
"publish()" use a new, empty queue, while the old queue is drained and
then discarded. Before doing this, I think new log records might get
added to the draining queue, if some logging occurred during draining.

Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
---
 .../traceeventlogger/SnapshotHandler.java     | 24 +++++++++++++------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/src/main/java/org/eclipse/tracecompass/traceeventlogger/SnapshotHandler.java b/src/main/java/org/eclipse/tracecompass/traceeventlogger/SnapshotHandler.java
index 743d396..77eace3 100644
--- a/src/main/java/org/eclipse/tracecompass/traceeventlogger/SnapshotHandler.java
+++ b/src/main/java/org/eclipse/tracecompass/traceeventlogger/SnapshotHandler.java
@@ -36,7 +36,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.logging.FileHandler;
-import java.util.logging.Level;
 import java.util.logging.LogManager;
 import java.util.logging.LogRecord;
 
@@ -143,8 +142,8 @@ private void configure() {
     @Override
     public boolean isLoggable(LogRecord logRecord) {
         // feature switch here
-        return (fIsEnabled && logRecord != null && super.isLoggable(logRecord)
-                && (logRecord.getLevel().intValue() <= Level.FINE.intValue()) && (logRecord instanceof TraceEventLogRecord)); // add
+        return (fIsEnabled && super.isLoggable(logRecord)
+                && (logRecord instanceof TraceEventLogRecord)); // add
     }
 
     private boolean addToSnapshot(LogRecord message) {
@@ -168,14 +167,26 @@ private boolean addToSnapshot(LogRecord message) {
         case "E": //$NON-NLS-1$
         {
             InnerEvent lastEvent = stack.remove(stack.size() - 1);
+            // top-level trace "segment" for this thread?
             if (stack.isEmpty()) {
+                // some housekeeping before flushing
+                pidMap.remove(event.getTid());
+                if (pidMap.isEmpty()) {
+                    // was the last entry for this pid/tid
+                    fStacks.remove(event.getPid());
+                }
+
                 // convert to seconds
                 double delta = (event.getTs() - lastEvent.getTs()) * 0.000001;
                 if (delta > fTimeout) {
+                    Deque<InnerEvent> toDrain = fData;
+                    // Start a new in-memory event queue, so we can continue to
+                    // process new events while draining the old queue to disk
+                    fData = new ArrayDeque<>();
                     if(fAsynchronousDrain) {
-                        drain(fData);
+                        drain(toDrain);
                     } else {
-                        drainTrace(fData).run();
+                        drainTrace(toDrain).run();
                     }
                 }
             }
@@ -189,10 +200,9 @@ private boolean addToSnapshot(LogRecord message) {
 
     @Override
     public synchronized void publish(LogRecord record) {
-        if (record != null) {
+        if (isLoggable(record)) {
             addToSnapshot(record);
         }
-        super.publish(record);
     }
 
     private void drain(Deque<InnerEvent> data) {