Introduce Blink-To-Append-Only Memoization and Apply to TableLoggers #4880

nbauernfeind · 2023-11-22T17:30:16Z

The primary purpose is to make PerformanceQueries work as intended (best-effort w.r.t. memory usage when not in use) according to existing documentation.

devinrsmith · 2023-11-22T18:21:00Z

engine/table/src/main/java/io/deephaven/engine/table/impl/BlinkTableTools.java

-        }
-    }
+            final QueryTable coalesced = (QueryTable) blinkTable.coalesce();

-    private static Table internalBlinkToAppendOnly(final Table blinkTable, long sizeLimit) {
-        return QueryPerformanceRecorder.withNugget("blinkToAppendOnly", () -> {
-            if (!isBlink(blinkTable)) {
+            if (!isBlink(coalesced)) {


I see the order has changed here - coalescing first then checking blink. Does it make more sense to keep the check first?

More generally, are we really using coalesce() b/c we need to make sure manage livenessscope stack stuff? If so, are we maybe abusing coalesce in this context b/c we already know a blinktable is coalesced? (And maybe, we should have a more appropriate method on Table for this situation?)

It would not be inappropriate to do the check before coalescing.

It's better to do the coalesce inside of the withUpdateGraph(), although that should be managed internally anyway.

The coalesce() is very much "belt and suspenders" hypothetical correctness in this case, as there are no uncoalesced blink tables in use.

What more appropriate method do you have in mind?

Good catch. That is my mistake. I needed the QueryTable to invoke getResult and I got the order wrong.

engine/table/src/main/java/io/deephaven/engine/table/impl/BlinkTableTools.java

engine/table/src/main/java/io/deephaven/engine/table/impl/MemoizedOperationKey.java

rcaudy

Mostly looking good. I wish you had separated out some of the refactoring to its own commit.

rcaudy · 2023-11-22T20:22:12Z

engine/table/src/main/java/io/deephaven/engine/table/impl/QueryTable.java

@@ -116,7 +116,7 @@ default boolean snapshotNeeded() {
         */
        class Result<T extends DynamicNode & NotificationStepReceiver> {
            public final T resultNode;
-            public final TableUpdateListener resultListener; // may be null if parent is non-ticking
+            public final TableUpdateListener resultListener; // may be null if parent or result are non-refreshing


When do we anticipate a non-refreshing result from a refreshing parent? snapshot(). Any others?

I see what you had in mind. I think this comment should be revised, but I can accept that there's a case for no listener.

In this case .. the append-only table could hit its size limit during initialization. Maybe this is an over optimization, but it does allow the result to be static. I can't think of anything other than a snapshot.

engine/table/src/main/java/io/deephaven/engine/table/impl/OperationSnapshotControl.java

engine/table/src/main/java/io/deephaven/engine/updategraph/impl/PeriodicUpdateGraph.java

engine/table/src/main/java/io/deephaven/engine/table/impl/BlinkTableTools.java

Co-authored-by: Ryan Caudy <rcaudy@gmail.com>

rcaudy

.

engine/table/src/main/java/io/deephaven/engine/table/impl/BlinkTableTools.java

rcaudy · 2023-11-23T01:28:43Z

engine/table/src/main/java/io/deephaven/engine/table/impl/BlinkTableTools.java

+    public static final Object DEFAULT_MEMO_KEY = new Object() {
+        @Override
+        public String toString() {
+            return "DEFAULT_MEMOIZATION_KEY";
+        }
+    };


Could just be

new String("DEFAULT_MEMOIZATION_KEY");

I'm a fan of how it's setup here w/ Object; even though unlikely, a string-based memo-key is "public" and the user can just use the same string.

I think that's an argument for using object identity when comparing memo keys. Also, why is it bad if the user really wants to explicitly use our default memo key? It's public here, for some reason, although I'd think it could be private.

Oh, I see it's explicitly public. That said, I still think it makes sense as object. (Only way to get default memo is to explicitly use this key.)

I don't think we'd want to use object identity for all memo, otherwise it wouldn't be obvious how one achieves memo over API today.

OK. I'm convinced enough. We stick to Object equality, keep this default as an Object so it will rely on reference equality, and maybe make it private (but I'm ambivalent about that).

engine/table/src/main/java/io/deephaven/engine/table/impl/BlinkTableTools.java

…me cases

deephaven-internal · 2023-11-24T21:54:49Z

Labels indicate documentation is required. Issues for documentation have been opened:

How-to: https://github.com/deephaven/deephaven.io/issues/3488
Reference: https://github.com/deephaven/deephaven.io/issues/3487

Introduce Blink-To-Append-Only Memoization and Apply to TableLoggers

469a6e5

nbauernfeind added query engine core Core development tasks DocumentationNeeded ReleaseNotesNeeded Release notes are needed labels Nov 22, 2023

nbauernfeind added this to the November 2023 milestone Nov 22, 2023

nbauernfeind requested review from rcaudy and devinrsmith November 22, 2023 17:30

nbauernfeind self-assigned this Nov 22, 2023

devinrsmith reviewed Nov 22, 2023

View reviewed changes

devinrsmith mentioned this pull request Nov 22, 2023

Add memoization for RingTableTools #4882

Open

rcaudy reviewed Nov 22, 2023

View reviewed changes

nbauernfeind and others added 2 commits November 22, 2023 16:06

Ryan's Suggestions

5130094

Co-authored-by: Ryan Caudy <rcaudy@gmail.com>

Review Feedback Rnd1

6b0a91d

nbauernfeind force-pushed the blink_to_append_memo branch from 469f8b7 to 6b0a91d Compare November 23, 2023 00:37

nbauernfeind requested review from devinrsmith and rcaudy November 23, 2023 00:38

rcaudy reviewed Nov 23, 2023

View reviewed changes

Rnd2 Feedback

33f651c

nbauernfeind requested a review from rcaudy November 24, 2023 20:33

rcaudy previously approved these changes Nov 24, 2023

View reviewed changes

devinrsmith requested changes Nov 24, 2023

View reviewed changes

engine/table/src/main/java/io/deephaven/engine/table/impl/BlinkTableTools.java Show resolved Hide resolved

revert back to public - ryan doesn't care, and it might be nice in so…

8934cd7

…me cases

nbauernfeind dismissed rcaudy’s stale review via 8934cd7 November 24, 2023 20:55

devinrsmith self-requested a review November 24, 2023 21:05

devinrsmith approved these changes Nov 24, 2023

View reviewed changes

nbauernfeind merged commit 8cfe8a8 into deephaven:main Nov 24, 2023
12 checks passed

github-actions bot locked and limited conversation to collaborators Nov 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Blink-To-Append-Only Memoization and Apply to TableLoggers #4880

Introduce Blink-To-Append-Only Memoization and Apply to TableLoggers #4880

nbauernfeind commented Nov 22, 2023

devinrsmith Nov 22, 2023

rcaudy Nov 22, 2023

nbauernfeind Nov 22, 2023

rcaudy left a comment

rcaudy Nov 22, 2023

rcaudy Nov 22, 2023

nbauernfeind Nov 22, 2023

rcaudy left a comment

rcaudy Nov 23, 2023

devinrsmith Nov 24, 2023

rcaudy Nov 24, 2023

devinrsmith Nov 24, 2023 •

edited

Loading

rcaudy Nov 24, 2023

deephaven-internal commented Nov 24, 2023

Introduce Blink-To-Append-Only Memoization and Apply to TableLoggers #4880

Introduce Blink-To-Append-Only Memoization and Apply to TableLoggers #4880

Conversation

nbauernfeind commented Nov 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcaudy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deephaven-internal commented Nov 24, 2023

devinrsmith Nov 24, 2023 •

edited

Loading