feat: delta updates #1069

wusteven815 · 2024-12-18T17:08:15Z

Closes Handle document delta updates sent from the server #123
- Generates and sends a JSON Patch (RFC 6902) instead of the entire document
- Diffs between new and old React objects to preserve unchanged old ones

mofojed · 2024-12-19T18:36:57Z

plugins/ui/setup.cfg

    json-rpc
+    pyjsonpatch


We should pin json-rpc and pyjsonpatch. Right now if either of these libraries update with a breaking change, users with new installs may not work correctly.

Suggested change

json-rpc

pyjsonpatch

json-rpc>=1.15.0

pyjsonpatch>=0.1.1

Docs on compatible release notation: https://peps.python.org/pep-0440/#compatible-release

mofojed · 2024-12-19T18:50:41Z

plugins/ui/src/js/src/widget/WidgetHandler.tsx

+        'connection' in newObj &&
+        'fetched' in newObj &&
+        'ticket_0' in newObj


This is a little sketchy

mofojed · 2024-12-19T18:56:02Z

plugins/ui/src/js/src/widget/WidgetHandler.tsx

+          log.debug2(METHOD_DOCUMENT_PATCHED, params);
+          const [patch, stateParam] = params;
+
+          applyPatch(uiDomRef.current, patch);


Ideally what we should do is only traverse the patch itself to know which elements have changed/need updates - rather than applying the patch and diffing afterwards. I think this can be achieved by iterating through the patch and using applyOperation instead of applyPatch.
I think we can also look at the value in the patch, and check if it's an object/element node or whatever, and replace that value before we actually call applyOperation.
Then in the returned result, take a look at removed for any objects we may need to cleanup/close.

mofojed · 2024-12-19T18:58:14Z

plugins/ui/src/js/src/widget/WidgetHandler.tsx

+  const reactDomRef: any = useRef(null); // eslint-disable-line @typescript-eslint/no-explicit-any
+  const reactDomNewRef: any = useRef(null); // eslint-disable-line @typescript-eslint/no-explicit-any


We shouldn't need to use any here - may need to add typing to the parseDocumentFromObject and use that typing for these.

wusteven815 · 2024-12-19T19:54:11Z

Here's the example I use to test around delta updates:

from deephaven import ui,empty_table
from random import randint
from time import time

counter = 0
_tables = []
for i in range(10):
    _tables.append(empty_table(i).update(f"X{i}=i"))

@ui.component
def delta_test():
    global counter
    items, set_items = ui.use_state([])
    i1, seti1 = ui.use_state(0)
    i2, seti2 = ui.use_state(0)
    colour, set_colour = ui.use_state([255, 255, 255])

    def handle_add():
        global counter
        num = counter
        set_items([*items, ui.button(f"{num}", on_press=lambda: print(num)), ui.table(_tables[num], width="80px")])
        counter += 1

    def handle_swap():
        items[i1], items[i2] = items[i2], items[i1]
        set_items([*items])

    return [
        ui.flex(
            ui.button("Add", on_press=handle_add),
            ui.button("Delete", on_press=lambda: set_items([*items[:-1]])),
            ui.button("Swap", on_press=handle_swap),
            ui.text_field(value=i1, on_change=lambda x: seti1(int(x)), width="80px"),
            ui.text_field(value=i2, on_change=lambda x: seti2(int(x)), width="80px"),
            ui.button("Colour", on_press=lambda: set_colour([randint(0, 255), randint(0, 255), randint(0, 255)])),
            ui.view(width="80px", background_color=f"rgb({colour[0]}, {colour[1]}, {colour[2]})",  key="test"),
            max_height="32px"
        ),
        ui.flex(*items, direction="row", flex_grow=1),
    ]

my_delta = delta_test()

mattrunyon

Didn't go too in depth on the logic as I assume some of this is going to change in a potentially significant way with a refactor

mattrunyon · 2024-12-20T17:19:16Z

plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py

        encoder_result = self._encoder.encode_node(root)
        encoded_document = encoder_result["encoded_node"]
        new_objects = encoder_result["new_objects"]
        callable_id_dict = encoder_result["callable_id_dict"]

+        document = json.loads(encoded_document)


We should see if we can do this before the node is stringified. Or is this necessary? Just don't want an unnecessary stringify -> parse -> stringify on the server

mattrunyon · 2024-12-20T17:22:21Z

plugins/ui/src/js/src/widget/WidgetHandler.test.tsx

 import WidgetHandler, { WidgetHandlerProps } from './WidgetHandler';
 import { DocumentHandlerProps } from './DocumentHandler';
 import {
  makeWidget,
  makeWidgetDescriptor,
-  makeWidgetEventDocumentUpdated,
+  makeWidgetEventDocumentPatched,


Idk if it's worth changing the name of this event on client/server from update to patch. Update seems fine still. Just reduces the size of the PR since there is no separate update action. Update might even be a clearer name to describe what is happening since patched could imply "fixed"

@mofojed thoughts? I know it's a bit pedantic and I'm fine leaving it as patch. Just thought I'd mention it

mattrunyon · 2024-12-20T17:28:49Z

plugins/ui/src/js/src/widget/WidgetHandler.test.tsx

@@ -210,7 +213,8 @@ it('updates the initial data only when widget has changed', async () => {
  expect(sendMessage).not.toHaveBeenCalled();

  const widget2 = makeWidgetDescriptor();
-  const document2 = { FOO: 'BAR' };
+  const document2 = { foo: 'bar', FOO: 'BAR' };
+  const patch2: Operation[] = [{ op: 'add', path: '/FOO', value: 'BAR' }];


We should test other operations too. I'm assuming there's remove and modify/change at least

mattrunyon · 2024-12-20T17:38:12Z

plugins/ui/src/js/src/widget/WidgetHandler.tsx

+  const uiDomRef = useRef({});
+  const reactDomRef: any = useRef(null); // eslint-disable-line @typescript-eslint/no-explicit-any
+  const reactDomNewRef: any = useRef(null); // eslint-disable-line @typescript-eslint/no-explicit-any


I would add comments what each of these are supposed to hold (if they stay after the suggested refactor to parse the patch instead of apply then parse special handling).

Also, minor thing, but in a render cycle I prefer to use old or prev or next instead of new. If it's tracking the previous and current state (or current and next for some less common reason).

With the current setup, these might make more sense as reactJSONRef and reactDomRef or something for the object and the parsed object

mattrunyon · 2024-12-20T17:45:08Z

plugins/ui/src/js/src/widget/WidgetHandler.tsx

+      const deepCopyAndParse = (obj: unknown, map = new WeakMap()): unknown => {
+        // make a deep copy of the object and recurse on children before making any replacements
+        if (obj === null || typeof obj !== 'object') return obj;
+        if (map.has(obj)) return map.get(obj);
+        const clone = Array.isArray(obj)
+          ? []
+          : Object.create(Object.getPrototypeOf(obj));
+        map.set(obj, clone);
+        const keys = Reflect.ownKeys(obj);
+        keys.forEach(key => {
+          const value = obj[key as keyof typeof obj];
+          clone[key] = deepCopyAndParse(value, map);
+        });


Guessing this will go away in refactor, but you can use something like lodash cloneDeepWith here instead. You can pass it an override function and if you don't return a value, it will handle cloning. So something like cloneDeepWith(obj, val => map.has(val) ? map.get(val) : undefined) I think would do it without tracking the found objects

mattrunyon · 2024-12-20T17:46:36Z

plugins/ui/src/js/src/widget/WidgetHandler.tsx

+      oldObj: any, // eslint-disable-line @typescript-eslint/no-explicit-any
+      newObj: any // eslint-disable-line @typescript-eslint/no-explicit-any
+      // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    ): { changed: boolean; obj: any } => {


Prefer unknown here if we don't know what it is

mattrunyon · 2024-12-20T17:49:38Z

plugins/ui/src/js/src/widget/WidgetHandler.tsx

+      if (changed) return { changed: true, obj: { ...obj } };
+      return { changed: false, obj: oldObj };
+    },
+    []


A callback with no dependencies should just be defined outside of the component. Also lets you break up the function a bit more into smaller, easily testable sections

wusteven815 added 2 commits December 18, 2024 12:06

add delta updates

1970f81

Merge branch 'deephaven:main' into 123-delta-updates

35de75d

wusteven815 self-assigned this Dec 18, 2024

fix tests

840b956

wusteven815 requested review from bmingles, mofojed, mattrunyon and jnumainville December 18, 2024 22:29

wusteven815 marked this pull request as ready for review December 19, 2024 14:26

mofojed requested changes Dec 19, 2024

View reviewed changes

mattrunyon requested changes Dec 20, 2024

View reviewed changes

mofojed self-assigned this Dec 31, 2024

bmingles removed their request for review January 2, 2025 14:50

mofojed mentioned this pull request Jan 22, 2025

docs: render cycle #1087

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: delta updates #1069

feat: delta updates #1069

wusteven815 commented Dec 18, 2024 •

edited

Loading

mofojed Dec 19, 2024

mofojed Dec 19, 2024

mofojed Dec 19, 2024

mofojed Dec 19, 2024

wusteven815 commented Dec 19, 2024

mattrunyon left a comment

mattrunyon Dec 20, 2024

mattrunyon Dec 20, 2024

mattrunyon Dec 20, 2024

mattrunyon Dec 20, 2024

mattrunyon Dec 20, 2024

mattrunyon Dec 20, 2024

mattrunyon Dec 20, 2024

		const reactDomRef: any = useRef(null); // eslint-disable-line @typescript-eslint/no-explicit-any
		const reactDomNewRef: any = useRef(null); // eslint-disable-line @typescript-eslint/no-explicit-any

feat: delta updates #1069

Are you sure you want to change the base?

feat: delta updates #1069

Conversation

wusteven815 commented Dec 18, 2024 • edited Loading

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

wusteven815 commented Dec 19, 2024

mattrunyon 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

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

wusteven815 commented Dec 18, 2024 •

edited

Loading