Non-invasive Rnd3 Feedback

deephaven · Nov 13, 2023 · 2ddb440 · 2ddb440
1 parent 9d00dd2
commit 2ddb440
Show file tree

Hide file tree

Showing 8 changed files with 53 additions and 32 deletions.
diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java
@@ -1269,7 +1269,7 @@ void handleUncaughtException(Exception throwable) {
                                                 initialFilterExecution.getBasePerformanceEntry();
                                         if (basePerformanceEntry != null) {
                                             final QueryPerformanceNugget outerNugget =
-                                                    QueryPerformanceRecorder.getInstance().getOuterNugget();
+                                                    QueryPerformanceRecorder.getInstance().getEnclosingNugget();
                                             if (outerNugget != null) {
                                                 outerNugget.accumulate(basePerformanceEntry);
                                             }
@@ -1517,7 +1517,7 @@ this, mode, columns, rowSet, getModifiedColumnSetForUpdates(), publishTheseSourc
                                 final BasePerformanceEntry baseEntry = jobScheduler.getAccumulatedPerformance();
                                 if (baseEntry != null) {
                                     final QueryPerformanceNugget outerNugget =
-                                            QueryPerformanceRecorder.getInstance().getOuterNugget();
+                                            QueryPerformanceRecorder.getInstance().getEnclosingNugget();
                                     if (outerNugget != null) {
                                         outerNugget.accumulate(baseEntry);
                                     }

diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/perf/BasePerformanceEntry.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/perf/BasePerformanceEntry.java
@@ -75,7 +75,8 @@ synchronized void baseEntryReset() {
     }
 
     /**
-     * Get the aggregate usage in nanoseconds. Invoking this getter is valid iff the entry will no longer be mutated.
+     * Get the aggregate usage in nanoseconds. This getter should be called by exclusive owners of the entry, and never
+     * concurrently with mutators.
      *
      * @return total wall clock time in nanos
      */
@@ -84,7 +85,8 @@ public long getUsageNanos() {
     }
 
     /**
-     * Get the aggregate cpu time in nanoseconds. Invoking this getter is valid iff the entry will no longer be mutated.
+     * Get the aggregate cpu time in nanoseconds. This getter should be called by exclusive owners of the entry, and
+     * never concurrently with mutators.
      *
      * @return total cpu time in nanos
      */
@@ -93,8 +95,8 @@ public long getCpuNanos() {
     }
 
     /**
-     * Get the aggregate cpu user time in nanoseconds. Invoking this getter is valid iff the entry will no longer be
-     * mutated.
+     * Get the aggregate cpu user time in nanoseconds. This getter should be called by exclusive owners of the entry,
+     * and never concurrently with mutators.
      *
      * @return total cpu user time in nanos
      */
@@ -103,8 +105,8 @@ public long getUserCpuNanos() {
     }
 
     /**
-     * Get the aggregate allocated memory in bytes. Invoking this getter is valid iff the entry will no longer be
-     * mutated.
+     * Get the aggregate allocated memory in bytes. This getter should be called by exclusive owners of the entry, and
+     * never concurrently with mutators.
      *
      * @return The bytes of allocated memory attributed to the instrumented operation.
      */
@@ -113,8 +115,8 @@ public long getAllocatedBytes() {
     }
 
     /**
-     * Get allocated pooled/reusable memory attributed to the instrumented operation in bytes. Invoking this getter is
-     * valid iff the entry will no longer be mutated.
+     * Get allocated pooled/reusable memory attributed to the instrumented operation in bytes. This getter should be
+     * called by exclusive owners of the entry, and never concurrently with mutators.
      *
      * @return total pool allocated memory in bytes
      */

diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/perf/QueryPerformanceNugget.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/perf/QueryPerformanceNugget.java
@@ -34,6 +34,16 @@ public class QueryPerformanceNugget extends BasePerformanceEntry implements Safe
         public void accumulate(@NotNull BasePerformanceEntry entry) {
             // non-synchronized no-op override
         }
+
+        @Override
+        public boolean shouldLogThisAndStackParents() {
+            return false;
+        }
+
+        @Override
+        boolean shouldLogNugget(boolean isUninstrumented) {
+            return false;
+        }
     };
 
     public interface Factory {
@@ -322,15 +332,16 @@ public boolean isUser() {
         return isUser;
     }
 
-    public boolean isBatchLevel() {
-        return isQueryLevel() && parentEvaluationNumber == NULL_LONG;
-    }
-
     public boolean isQueryLevel() {
         return operationNumber == NULL_INT;
     }
 
-    public boolean isTopLevel() {
+    public boolean isTopLevelQuery() {
+        return isQueryLevel() && parentEvaluationNumber == NULL_LONG;
+    }
+
+    public boolean isTopLevelOperation() {
+        // note that query level nuggets have depth == NULL_INT
         return depth == 0;
     }
 
@@ -416,15 +427,15 @@ public boolean wasInterrupted() {
     /**
      * Ensure this nugget gets logged, alongside its stack of nesting operations.
      */
-    public void setShouldLogThisAndStackParents() {
+    void setShouldLogThisAndStackParents() {
         shouldLogThisAndStackParents = true;
     }
 
     /**
      * @return true if this nugget triggers the logging of itself and every other nugget in its stack of nesting
      *         operations.
      */
-    public boolean shouldLogThisAndStackParents() {
+    boolean shouldLogThisAndStackParents() {
         return shouldLogThisAndStackParents;
     }
 

diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/perf/QueryPerformanceRecorder.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/perf/QueryPerformanceRecorder.java
@@ -82,22 +82,32 @@ public static void resetInstance() {
     }
 
     /**
+     * Create a nugget at the top of the user stack. May return a {@link QueryPerformanceNugget#DUMMY_NUGGET} if no
+     * recorder is installed.
+     *
      * @param name the nugget name
-     * @return A new QueryPerformanceNugget to encapsulate user query operations. done() must be called on the nugget.
+     * @return A new QueryPerformanceNugget to encapsulate user query operations. {@link QueryPerformanceNugget#done()}
+     *         or {@link QueryPerformanceNugget#close()} must be called on the nugget.
      */
     public abstract QueryPerformanceNugget getNugget(@NotNull String name);
 
     /**
+     * Create a nugget at the top of the user stack. May return a {@link QueryPerformanceNugget#DUMMY_NUGGET} if no
+     * recorder is installed.
+     *
      * @param name the nugget name
      * @param inputSize the nugget's input size
-     * @return A new QueryPerformanceNugget to encapsulate user query operations. done() must be called on the nugget.
+     * @return A new QueryPerformanceNugget to encapsulate user query operations. {@link QueryPerformanceNugget#done()}
+     *         or {@link QueryPerformanceNugget#close()} must be called on the nugget.
      */
     public abstract QueryPerformanceNugget getNugget(@NotNull String name, long inputSize);
 
     /**
-     * @return The nugget currently in effect or else a dummy nugget if no nugget is in effect.
+     * This is the nugget enclosing the current operation. It may belong to the dummy recorder, or a real one.
+     *
+     * @return Either a "catch-all" nugget, or the top of the user nugget stack.
      */
-    public abstract QueryPerformanceNugget getOuterNugget();
+    public abstract QueryPerformanceNugget getEnclosingNugget();
 
     /**
      * <b>Note:</b> Do not call this directly - it's for nugget use only. Call {@link QueryPerformanceNugget#done()} or
@@ -462,7 +472,7 @@ public QueryPerformanceNugget getNugget(@NotNull final String name, long inputSi
         }
 
         @Override
-        public QueryPerformanceNugget getOuterNugget() {
+        public QueryPerformanceNugget getEnclosingNugget() {
             return QueryPerformanceNugget.DUMMY_NUGGET;
         }
 

diff --git a/...table/src/main/java/io/deephaven/engine/table/impl/perf/QueryPerformanceRecorderImpl.java b/...table/src/main/java/io/deephaven/engine/table/impl/perf/QueryPerformanceRecorderImpl.java
@@ -265,7 +265,7 @@ synchronized boolean releaseNugget(@NotNull final QueryPerformanceNugget nugget)
     }
 
     @Override
-    public synchronized QueryPerformanceNugget getOuterNugget() {
+    public synchronized QueryPerformanceNugget getEnclosingNugget() {
         return userNuggetStack.peekLast();
     }
 
@@ -321,7 +321,7 @@ public synchronized Table getTimingResultsAsTable() {
             timeNanos[i] = operationNuggets.get(i).getUsageNanos();
             names[i] = operationNuggets.get(i).getName();
             callerLine[i] = operationNuggets.get(i).getCallerLine();
-            isTopLevel[i] = operationNuggets.get(i).isTopLevel();
+            isTopLevel[i] = operationNuggets.get(i).isTopLevelOperation();
             isCompileTime[i] = operationNuggets.get(i).getName().startsWith("Compile:");
         }
         return TableTools.newTable(

diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/perf/QueryProcessingResults.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/perf/QueryProcessingResults.java
@@ -3,11 +3,7 @@
  */
 package io.deephaven.engine.table.impl.perf;
 
-import java.io.Serializable;
-
-public class QueryProcessingResults implements Serializable {
-
-    private static final long serialVersionUID = 2L;
+public class QueryProcessingResults {
 
     private final QueryPerformanceRecorder recorder;
 

diff --git a/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateBy.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/updateby/UpdateBy.java
@@ -907,7 +907,8 @@ private void cleanUpAndNotify(final Runnable onCleanupComplete) {
             final BasePerformanceEntry accumulated = jobScheduler.getAccumulatedPerformance();
             if (accumulated != null) {
                 if (initialStep) {
-                    final QueryPerformanceNugget outerNugget = QueryPerformanceRecorder.getInstance().getOuterNugget();
+                    final QueryPerformanceNugget outerNugget =
+                            QueryPerformanceRecorder.getInstance().getEnclosingNugget();
                     if (outerNugget != null) {
                         outerNugget.accumulate(accumulated);
                     }

diff --git a/server/src/main/java/io/deephaven/server/table/ops/TableServiceGrpcImpl.java b/server/src/main/java/io/deephaven/server/table/ops/TableServiceGrpcImpl.java
@@ -550,8 +550,9 @@ public void batch(
                     } else {
                         safelyComplete(responseObserver);
                     }
-                    queryPerformanceRecorder.endQuery();
-                    EngineMetrics.getInstance().logQueryProcessingResults(results);
+                    if (queryPerformanceRecorder.endQuery()) {
+                        EngineMetrics.getInstance().logQueryProcessingResults(results);
+                    }
                 } finally {
                     QueryPerformanceRecorder.resetInstance();
                 }