From 2ddb4409188e819b6fa8df64862a1faa16a10880 Mon Sep 17 00:00:00 2001 From: Nathaniel Bauernfeind Date: Mon, 13 Nov 2023 11:10:23 -0700 Subject: [PATCH] Non-invasive Rnd3 Feedback --- .../engine/table/impl/QueryTable.java | 4 +-- .../table/impl/perf/BasePerformanceEntry.java | 18 +++++++------ .../impl/perf/QueryPerformanceNugget.java | 25 +++++++++++++------ .../impl/perf/QueryPerformanceRecorder.java | 20 +++++++++++---- .../perf/QueryPerformanceRecorderImpl.java | 4 +-- .../impl/perf/QueryProcessingResults.java | 6 +---- .../engine/table/impl/updateby/UpdateBy.java | 3 ++- .../table/ops/TableServiceGrpcImpl.java | 5 ++-- 8 files changed, 53 insertions(+), 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 index b43d6531d61..8d78c5ef5d4 100644 --- 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 index e9aae8c9a4a..40586105d58 100644 --- 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 index 35eb3b63d41..92aa7d8889b 100644 --- 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,7 +427,7 @@ public boolean wasInterrupted() { /** * Ensure this nugget gets logged, alongside its stack of nesting operations. */ - public void setShouldLogThisAndStackParents() { + void setShouldLogThisAndStackParents() { shouldLogThisAndStackParents = true; } @@ -424,7 +435,7 @@ public void setShouldLogThisAndStackParents() { * @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 index 856d695d21a..549dfb2c9a0 100644 --- 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(); /** * Note: 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/engine/table/src/main/java/io/deephaven/engine/table/impl/perf/QueryPerformanceRecorderImpl.java b/engine/table/src/main/java/io/deephaven/engine/table/impl/perf/QueryPerformanceRecorderImpl.java index 7b1f868023e..3c70db1d0e3 100644 --- a/engine/table/src/main/java/io/deephaven/engine/table/impl/perf/QueryPerformanceRecorderImpl.java +++ b/engine/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 index 6bdc697b598..41b04ff7a8f 100644 --- 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 index e603905e5c7..a9f1a4561ae 100644 --- 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 index 20890529091..e7784812e9e 100644 --- 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(); }