From 09d6c16714a9eb263d8224da387bb7bccb8d5f6d Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Mon, 20 Nov 2023 23:02:02 -0500 Subject: [PATCH 01/11] WIP Only send newly exported objects - Client retains a reference to previously exported objects if they're the same object - Client builds the map of IDs to exported objects when each new documentUpdated event is received - A strong reference is retained to all the exported objects in the current render so they do not go out of scope unexpectedly - A weak reference is stored from the ID to the exported object - Not tested at all --- .../ui/object_types/ElementMessageStream.py | 17 ++--- .../src/deephaven/ui/renderer/NodeEncoder.py | 75 +++++++++++-------- plugins/ui/src/js/src/WidgetHandler.tsx | 52 ++++++++++--- 3 files changed, 92 insertions(+), 52 deletions(-) diff --git a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py index 6448f9f43..1d052c2c3 100644 --- a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py +++ b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py @@ -25,7 +25,6 @@ def __init__(self, element: Element, connection: MessageStream): """ self._element = element self._connection = connection - self._update_count = 0 self._message_id = 0 self._manager = JSONRPCResponseManager() self._dispatcher = Dispatcher() @@ -98,22 +97,16 @@ def _send_document_update(self, root: RenderedNode) -> None: Args: root: The root node of the document to send """ - # We use an ID prefix to ensure that the callable ids are unique across each document render/update - # That way we don't have to worry about callables from previous renders being called accidentally - self._update_count += 1 - id_prefix = f"cb_{self._update_count}_" - # TODO(#67): Send a diff of the document instead of the entire document. request = self._make_notification("documentUpdated", root) - encoder = NodeEncoder(callable_id_prefix=id_prefix, separators=(",", ":")) + encoder = NodeEncoder(separators=(",", ":")) payload = encoder.encode(request) logger.debug(f"Sending payload: {payload}") dispatcher = Dispatcher() - for i, callable in enumerate(encoder.callables): - key = f"{id_prefix}{i}" - logger.debug("Registering callable %s", key) - dispatcher[key] = callable + for cb, callable_id in encoder.callable_dict.items(): + logger.debug("Registering callable %s", callable_id) + dispatcher[callable_id] = cb self._dispatcher = dispatcher - self._connection.on_data(payload.encode(), encoder.objects) + self._connection.on_data(payload.encode(), encoder.new_objects) diff --git a/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py b/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py index c7730d45e..791bf28c2 100644 --- a/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py +++ b/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py @@ -3,12 +3,21 @@ from collections.abc import Iterator import json from typing import Any, Callable +from weakref import WeakKeyDictionary, WeakValueDictionary from .RenderedNode import RenderedNode CALLABLE_KEY = "__dhCbid" OBJECT_KEY = "__dhObid" ELEMENT_KEY = "__dhElemName" +CALLABLE_ID_PREFIX = "cb_" + +# IDs for callables are prefixes with a string and then use the `id` of the callable itself +CallableId = str + +# IDs for objects is just an incrementing ID. We should only send new exported objects with each render +ObjectId = int + class NodeEncoder(json.JSONEncoder): """ @@ -18,46 +27,45 @@ class NodeEncoder(json.JSONEncoder): - non-serializable objects in the tree are replaced wtih an object with property `OBJECT_KEY` set to the index in the objects array. """ - _callable_id_prefix: str + _next_callable_id: int """ Prefix to use for callable ids. Used to ensure callables used in stream are unique. """ - _callables: list[Callable] + _callable_dict: WeakKeyDictionary[Callable, str] """ - List of callables parsed out of the document + Dictionary from a callable to the ID assigned to the callable. """ - _callable_id_dict: dict[int, int] + _new_objects: list[Any] """ - Dictionary from a callables id to the index in the callables array. + List of objects parsed out of the most recently encoded document. """ - _objects: list[Any] + _next_object_id: int """ - List of objects parsed out of the document + The next object id to use. Increment for each new object encountered. """ - _object_id_dict: dict[int, int] + _object_id_dict: WeakKeyDictionary[Any, int] """ - Dictionary from an objects id to the index in the objects array. + Dictionary from an object to the ID assigned to it """ - def __init__(self, *args, callable_id_prefix: str = "cb", **kwargs): + def __init__(self, *args, **kwargs): """ Create a new NodeEncoder. Args: *args: Arguments to pass to the JSONEncoder constructor - callable_id_prefix: Prefix to use for callable ids. Used to ensure callables used in stream are unique. **kwargs: Args to pass to the JSONEncoder constructor """ super().__init__(*args, **kwargs) - self._callable_id_prefix = callable_id_prefix - self._callables = [] - self._callable_id_dict = {} - self._objects = [] - self._object_id_dict = {} + self._next_callable_id = 0 + self._callable_dict = WeakValueDictionary() + self._new_objects = [] + self._next_object_id = 0 + self._object_id_dict = WeakKeyDictionary() def default(self, node: Any): if isinstance(node, RenderedNode): @@ -71,13 +79,17 @@ def default(self, node: Any): # This is a non-serializable object. We'll store a reference to the object in the objects array. return self._convert_object(node) + def encode(self, o: Any) -> str: + self._new_objects = [] + return super().encode(o) + @property - def callables(self) -> list[Callable]: + def callable_dict(self) -> WeakValueDictionary[Callable, str]: return self._callables @property - def objects(self) -> list[Any]: - return self._objects + def new_objects(self) -> list[Any]: + return self._new_objects def _convert_rendered_node(self, node: RenderedNode): result = {ELEMENT_KEY: node.name} @@ -86,23 +98,24 @@ def _convert_rendered_node(self, node: RenderedNode): return result def _convert_callable(self, cb: callable): - callable_id = id(cb) - callable_index = self._callable_id_dict.get(callable_id, len(self._callables)) - if callable_index == len(self._callables): - self._callables.append(cb) - self._callable_id_dict[callable_id] = callable_index + callable_id = self._callable_dict.get(cb) + if callable_id == None: + callable_id = f"{CALLABLE_ID_PREFIX}{self._next_callable_id}" + self._next_callable_id += 1 + self._callable_dict[cb] = callable_id return { - CALLABLE_KEY: f"{self._callable_id_prefix}{callable_index}", + CALLABLE_KEY: callable_id, } def _convert_object(self, obj: Any): - object_id = id(obj) - object_index = self._object_id_dict.get(object_id, len(self._objects)) - if object_index == len(self._objects): - self._objects.append(obj) - self._object_id_dict[object_id] = object_index + object_id = self._object_id_dict.get(obj) + if object_id == None: + object_id = self._next_object_id + self._next_object_id += 1 + self._object_id_dict[obj] = object_id + self._new_objects.append(obj) return { - OBJECT_KEY: object_index, + OBJECT_KEY: object_id, } diff --git a/plugins/ui/src/js/src/WidgetHandler.tsx b/plugins/ui/src/js/src/WidgetHandler.tsx index ffdc64fa3..421f4b2fe 100644 --- a/plugins/ui/src/js/src/WidgetHandler.tsx +++ b/plugins/ui/src/js/src/WidgetHandler.tsx @@ -1,7 +1,13 @@ /** * Handles document events for one widget. */ -import React, { useCallback, useEffect, useMemo, useState } from 'react'; +import React, { + useCallback, + useEffect, + useMemo, + useRef, + useState, +} from 'react'; import { JSONRPCClient, JSONRPCServer, @@ -35,6 +41,13 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { const [widget, setWidget] = useState(); const [element, setElement] = useState(); + // We keep a weak reference to all the exported objects so they can be garbage collected when we no longer need them + const exportedObjectMap = useRef>>( + new Map() + ); + const exportedObjectCount = useRef(0); + // We keep a ref to all the exported objects in the current render so they are _not_ garbage collected + const liveObjects = useRef([]); useEffect( () => () => { @@ -65,11 +78,11 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { * Element nodes are not replaced. Those are handled in `DocumentHandler`. * * @param data The data to parse - * @param exportedObjects The exported objects to use for re-hydrating objects * @returns The parsed data */ - (data: string, exportedObjects: WidgetExportedObject[]) => - JSON.parse(data, (key, value) => { + (data: string) => { + const newLiveObjects: WidgetExportedObject[] = []; + const parsedData = JSON.parse(data, (key, value) => { // Need to re-hydrate any objects that are defined if (isCallableNode(value)) { const callableId = value[CALLABLE_KEY]; @@ -81,11 +94,23 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { } if (isObjectNode(value)) { // Replace this node with the exported object - return exportedObjects[value[OBJECT_KEY]]; + const objectKey = value[OBJECT_KEY]; + const exportedObject = exportedObjectMap.current + .get(objectKey) + ?.deref(); + if (exportedObject === undefined) { + // The map should always have the exported object for a key, otherwise the protocol is broken + throw new Error(`Invalid exported object key ${objectKey}`); + } + newLiveObjects.push(exportedObject); + return exportedObject; } return value; - }), + }); + liveObjects.current = newLiveObjects; + return parsedData; + }, [jsonClient] ); @@ -114,10 +139,19 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { } function receiveData( data: string, - exportedObjects: WidgetExportedObject[] + newExportedObjects: WidgetExportedObject[] ) { - log.debug2('Data received', data, exportedObjects); - const parsedData = parseData(data, exportedObjects); + log.debug2('Data received', data, newExportedObjects); + for (let i = 0; i < newExportedObjects.length; i += 1) { + const exportedObject = newExportedObjects[i]; + const exportedObjectKey = exportedObjectCount.current; + exportedObjectCount.current += 1; + exportedObjectMap.current.set( + exportedObjectKey, + new WeakRef(exportedObject) + ); + } + const parsedData = parseData(data); jsonClient?.receiveAndSend(parsedData); } From 504fe35e82c3ff92ec2ae363435332b230f75a00 Mon Sep 17 00:00:00 2001 From: mikebender Date: Tue, 21 Nov 2023 09:51:45 -0500 Subject: [PATCH 02/11] Update unit tests and clean up code - Added some missing types in Element and RenderedNode - Added unit tests to verify that objects/callables stay valid after updates --- .../ui/src/deephaven/ui/elements/Element.py | 6 +- .../ui/src/deephaven/ui/elements/__init__.py | 4 +- .../src/deephaven/ui/renderer/NodeEncoder.py | 41 ++--- .../src/deephaven/ui/renderer/RenderedNode.py | 8 +- plugins/ui/test/deephaven/ui/test_encoder.py | 140 +++++++++++++++++- 5 files changed, 172 insertions(+), 27 deletions(-) diff --git a/plugins/ui/src/deephaven/ui/elements/Element.py b/plugins/ui/src/deephaven/ui/elements/Element.py index c889b6459..e0c00ad0c 100644 --- a/plugins/ui/src/deephaven/ui/elements/Element.py +++ b/plugins/ui/src/deephaven/ui/elements/Element.py @@ -1,9 +1,11 @@ from __future__ import annotations from abc import ABC, abstractmethod -from typing import Any +from typing import Any, Dict from .._internal import RenderContext +PropsType = Dict[str, Any] + class Element(ABC): """ @@ -21,7 +23,7 @@ def name(self) -> str: return "deephaven.ui.Element" @abstractmethod - def render(self, context: RenderContext) -> dict[str, Any]: + def render(self, context: RenderContext) -> PropsType: """ Renders this element, and returns the result as a dictionary of props for the element. If you just want to render children, pass back a dict with children only, e.g. { "children": ... } diff --git a/plugins/ui/src/deephaven/ui/elements/__init__.py b/plugins/ui/src/deephaven/ui/elements/__init__.py index 1fada21ca..9003ffad9 100644 --- a/plugins/ui/src/deephaven/ui/elements/__init__.py +++ b/plugins/ui/src/deephaven/ui/elements/__init__.py @@ -1,6 +1,6 @@ -from .Element import Element +from .Element import Element, PropsType from .BaseElement import BaseElement from .FunctionElement import FunctionElement from .UITable import UITable -__all__ = ["BaseElement", "Element", "FunctionElement", "UITable"] +__all__ = ["BaseElement", "Element", "FunctionElement", "PropsType", "UITable"] diff --git a/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py b/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py index 791bf28c2..50cb10e8b 100644 --- a/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py +++ b/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py @@ -1,16 +1,15 @@ from __future__ import annotations -from collections.abc import Iterator import json from typing import Any, Callable -from weakref import WeakKeyDictionary, WeakValueDictionary +from weakref import WeakKeyDictionary from .RenderedNode import RenderedNode CALLABLE_KEY = "__dhCbid" OBJECT_KEY = "__dhObid" ELEMENT_KEY = "__dhElemName" -CALLABLE_ID_PREFIX = "cb_" +DEFALT_CALLABLE_ID_PREFIX = "cb" # IDs for callables are prefixes with a string and then use the `id` of the callable itself CallableId = str @@ -32,7 +31,7 @@ class NodeEncoder(json.JSONEncoder): Prefix to use for callable ids. Used to ensure callables used in stream are unique. """ - _callable_dict: WeakKeyDictionary[Callable, str] + _callable_dict: WeakKeyDictionary[Callable[..., Any], str] """ Dictionary from a callable to the ID assigned to the callable. """ @@ -52,7 +51,12 @@ class NodeEncoder(json.JSONEncoder): Dictionary from an object to the ID assigned to it """ - def __init__(self, *args, **kwargs): + def __init__( + self, + callable_id_prefix: str = DEFALT_CALLABLE_ID_PREFIX, + *args: Any, + **kwargs: Any, + ): """ Create a new NodeEncoder. @@ -61,46 +65,47 @@ def __init__(self, *args, **kwargs): **kwargs: Args to pass to the JSONEncoder constructor """ super().__init__(*args, **kwargs) + self._callable_id_prefix = callable_id_prefix self._next_callable_id = 0 - self._callable_dict = WeakValueDictionary() + self._callable_dict = WeakKeyDictionary() self._new_objects = [] self._next_object_id = 0 self._object_id_dict = WeakKeyDictionary() - def default(self, node: Any): - if isinstance(node, RenderedNode): - return self._convert_rendered_node(node) - elif callable(node): - return self._convert_callable(node) + def default(self, o: Any): + if isinstance(o, RenderedNode): + return self._convert_rendered_node(o) + elif callable(o): + return self._convert_callable(o) else: try: - return super().default(node) + return super().default(o) except TypeError: # This is a non-serializable object. We'll store a reference to the object in the objects array. - return self._convert_object(node) + return self._convert_object(o) def encode(self, o: Any) -> str: self._new_objects = [] return super().encode(o) @property - def callable_dict(self) -> WeakValueDictionary[Callable, str]: - return self._callables + def callable_dict(self) -> WeakKeyDictionary[Callable[..., Any], str]: + return self._callable_dict @property def new_objects(self) -> list[Any]: return self._new_objects def _convert_rendered_node(self, node: RenderedNode): - result = {ELEMENT_KEY: node.name} + result: dict[str, Any] = {ELEMENT_KEY: node.name} if node.props is not None: result["props"] = node.props return result - def _convert_callable(self, cb: callable): + def _convert_callable(self, cb: Callable[..., Any]): callable_id = self._callable_dict.get(cb) if callable_id == None: - callable_id = f"{CALLABLE_ID_PREFIX}{self._next_callable_id}" + callable_id = f"{self._callable_id_prefix}{self._next_callable_id}" self._next_callable_id += 1 self._callable_dict[cb] = callable_id diff --git a/plugins/ui/src/deephaven/ui/renderer/RenderedNode.py b/plugins/ui/src/deephaven/ui/renderer/RenderedNode.py index 809a7c8fa..db9b71555 100644 --- a/plugins/ui/src/deephaven/ui/renderer/RenderedNode.py +++ b/plugins/ui/src/deephaven/ui/renderer/RenderedNode.py @@ -1,4 +1,6 @@ from __future__ import annotations +from typing import Optional +from ..elements import PropsType class RenderedNode: @@ -7,9 +9,9 @@ class RenderedNode: """ _name: str - _props: dict | None + _props: Optional[PropsType] - def __init__(self, name: str, props: dict = None): + def __init__(self, name: str, props: Optional[PropsType] = None): """ Stores the result of a rendered node @@ -28,7 +30,7 @@ def name(self) -> str: return self._name @property - def props(self) -> dict | None: + def props(self) -> Optional[PropsType]: """ Get the props of the node. """ diff --git a/plugins/ui/test/deephaven/ui/test_encoder.py b/plugins/ui/test/deephaven/ui/test_encoder.py index 2c8854d10..48e2e18b2 100644 --- a/plugins/ui/test/deephaven/ui/test_encoder.py +++ b/plugins/ui/test/deephaven/ui/test_encoder.py @@ -35,9 +35,13 @@ def expect_result( json.loads(payload), expected_payload, "payloads don't match" ) self.assertListEqual( - encoder.callables, expected_callables, "callables don't match" + list(encoder.callable_dict.keys()), + expected_callables, + "callables don't match", + ) + self.assertListEqual( + encoder.new_objects, expected_objects, "objects don't match" ) - self.assertListEqual(encoder.objects, expected_objects, "objects don't match") def test_empty_document(self): self.expect_result(make_node(""), {"__dhElemName": ""}) @@ -354,6 +358,138 @@ def test_callable_id_prefix(self): callable_id_prefix="d2c", ) + def test_delta_objects(self): + """ + Test that the encoder retains IDs for objects and callables that are the same between encodings. + Also check that the encoder only sends new objects in the most recent encoding. + """ + + from deephaven.ui.renderer import NodeEncoder + + objs = [TestObject(), TestObject(), TestObject()] + cbs = [lambda: None, lambda: None, lambda: None] + + # Should use a depth-first traversal to find all exported objects and their indices + encoder = NodeEncoder() + node = make_node( + "test0", + { + "children": [ + make_node("test1", {"foo": [cbs[0]]}), + cbs[1], + make_node("test3", {"bar": cbs[2], "children": [objs[0], objs[1]]}), + make_node("test4", {"children": [objs[0], objs[2]]}), + objs[1], + make_node("test6", {"children": [objs[2]]}), + ] + }, + ) + + payload = encoder.encode(node) + expected_payload = { + "__dhElemName": "test0", + "props": { + "children": [ + { + "__dhElemName": "test1", + "props": {"foo": [{"__dhCbid": "cb0"}]}, + }, + {"__dhCbid": "cb1"}, + { + "__dhElemName": "test3", + "props": { + "bar": {"__dhCbid": "cb2"}, + "children": [ + {"__dhObid": 0}, + {"__dhObid": 1}, + ], + }, + }, + { + "__dhElemName": "test4", + "props": {"children": [{"__dhObid": 0}, {"__dhObid": 2}]}, + }, + {"__dhObid": 1}, + { + "__dhElemName": "test6", + "props": {"children": [{"__dhObid": 2}]}, + }, + ], + }, + } + + self.assertDictEqual( + json.loads(payload), expected_payload, "payloads don't match" + ) + self.assertListEqual( + list(encoder.callable_dict.keys()), cbs, "callables don't match" + ) + self.assertListEqual(encoder.new_objects, objs, "objects don't match") + + # Add some new objects and callables to the mix for next encoding + delta_objs = [TestObject()] + delta_cbs = [lambda: None] + objs = [objs[0], None, objs[2], delta_objs[0]] + cbs = [cbs[0], None, cbs[2], delta_cbs[0]] + + # Replace cb[1] and obj[1] with the new callable/object and encode again + node = make_node( + "test0", + { + "children": [ + make_node("test1", {"foo": [cbs[0]]}), + cbs[3], + make_node("test3", {"bar": cbs[2], "children": [objs[0], objs[3]]}), + make_node("test4", {"children": [objs[0], objs[2]]}), + objs[3], + make_node("test6", {"children": [objs[2]]}), + ] + }, + ) + + payload = encoder.encode(node) + expected_payload = { + "__dhElemName": "test0", + "props": { + "children": [ + { + "__dhElemName": "test1", + "props": {"foo": [{"__dhCbid": "cb0"}]}, + }, + {"__dhCbid": "cb3"}, + { + "__dhElemName": "test3", + "props": { + "bar": {"__dhCbid": "cb2"}, + "children": [ + {"__dhObid": 0}, + {"__dhObid": 3}, + ], + }, + }, + { + "__dhElemName": "test4", + "props": {"children": [{"__dhObid": 0}, {"__dhObid": 2}]}, + }, + {"__dhObid": 3}, + { + "__dhElemName": "test6", + "props": {"children": [{"__dhObid": 2}]}, + }, + ], + }, + } + + self.assertDictEqual( + json.loads(payload), expected_payload, "payloads don't match" + ) + self.assertListEqual( + list(encoder.callable_dict.keys()), + [cbs[0], cbs[2], cbs[3]], + "callables don't match", + ) + self.assertListEqual(encoder.new_objects, delta_objs, "objects don't match") + if __name__ == "__main__": unittest.main() From a4c43775546bb2f4ed362d4f65106b876b6732be Mon Sep 17 00:00:00 2001 From: mikebender Date: Tue, 21 Nov 2023 11:48:44 -0500 Subject: [PATCH 03/11] Clean up after some self-review --- .../ui/object_types/ElementMessageStream.py | 16 ++++++++-------- .../ui/src/deephaven/ui/renderer/NodeEncoder.py | 4 ++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py index 1d052c2c3..2cd23cc07 100644 --- a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py +++ b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py @@ -1,5 +1,4 @@ from __future__ import annotations -import json import io from jsonrpc import JSONRPCResponseManager, Dispatcher import logging @@ -28,6 +27,7 @@ def __init__(self, element: Element, connection: MessageStream): self._message_id = 0 self._manager = JSONRPCResponseManager() self._dispatcher = Dispatcher() + self._encoder = NodeEncoder(separators=(",", ":")) def start(self) -> None: context = RenderContext() @@ -61,7 +61,7 @@ def _get_next_message_id(self) -> int: self._message_id += 1 return self._message_id - def _make_notification(self, method: str, *params: list[Any]) -> None: + def _make_notification(self, method: str, *params: Any) -> dict[str, Any]: """ Make a JSON-RPC notification. Can notify the client without expecting a response. @@ -75,7 +75,7 @@ def _make_notification(self, method: str, *params: list[Any]) -> None: "params": params, } - def _make_request(self, method: str, *params: list[Any]) -> None: + def _make_request(self, method: str, *params: Any) -> dict[str, Any]: """ Make a JSON-RPC request. Messages the client and expects a response. @@ -97,16 +97,16 @@ def _send_document_update(self, root: RenderedNode) -> None: Args: root: The root node of the document to send """ + # TODO(#67): Send a diff of the document instead of the entire document. request = self._make_notification("documentUpdated", root) - encoder = NodeEncoder(separators=(",", ":")) - payload = encoder.encode(request) + payload = self._encoder.encode(request) logger.debug(f"Sending payload: {payload}") dispatcher = Dispatcher() - for cb, callable_id in encoder.callable_dict.items(): + for callable, callable_id in self._encoder.callable_dict.items(): logger.debug("Registering callable %s", callable_id) - dispatcher[callable_id] = cb + dispatcher[callable_id] = callable self._dispatcher = dispatcher - self._connection.on_data(payload.encode(), encoder.new_objects) + self._connection.on_data(payload.encode(), self._encoder.new_objects) diff --git a/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py b/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py index 50cb10e8b..acdb12632 100644 --- a/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py +++ b/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py @@ -9,7 +9,7 @@ OBJECT_KEY = "__dhObid" ELEMENT_KEY = "__dhElemName" -DEFALT_CALLABLE_ID_PREFIX = "cb" +DEFAULT_CALLABLE_ID_PREFIX = "cb" # IDs for callables are prefixes with a string and then use the `id` of the callable itself CallableId = str @@ -53,7 +53,7 @@ class NodeEncoder(json.JSONEncoder): def __init__( self, - callable_id_prefix: str = DEFALT_CALLABLE_ID_PREFIX, + callable_id_prefix: str = DEFAULT_CALLABLE_ID_PREFIX, *args: Any, **kwargs: Any, ): From 8c4474facf8ded98507a77bf47f17a78796d825d Mon Sep 17 00:00:00 2001 From: mikebender Date: Tue, 21 Nov 2023 15:41:39 -0500 Subject: [PATCH 04/11] A little cleanup from self-review --- plugins/ui/DESIGN.md | 36 +++++++++---------- .../deephaven/ui/components/make_component.py | 9 +++-- .../ui/object_types/ElementMessageStream.py | 30 ++++++++++++++++ 3 files changed, 54 insertions(+), 21 deletions(-) diff --git a/plugins/ui/DESIGN.md b/plugins/ui/DESIGN.md index 8ec65fd3a..477718abe 100644 --- a/plugins/ui/DESIGN.md +++ b/plugins/ui/DESIGN.md @@ -1438,13 +1438,13 @@ use_table_listener( ###### Parameters -| Parameter | Type | Description | -|---------------|--------------------------------------------------------|-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| -| `table` | `Table` | The table to listen to. | -| `listener` | `Callable[[TableUpdate, bool], None] \| TableListener` | Either a function or a [TableListener](https://deephaven.io/core/pydoc/code/deephaven.table_listener.html#deephaven.table_listener.TableListener) with an on_update function. The function must take a [TableUpdate](https://deephaven.io/core/pydoc/code/deephaven.table_listener.html#deephaven.table_listener.TableUpdate) and is_replay bool. [More table listener info](https://deephaven.io/core/docs/how-to-guides/table-listeners-python/) | -| `description` | `str \| None` | An optional description for the UpdatePerformanceTracker to append to the listener’s entry description, default is None. -| `do_replay` | `bool` | Whether to replay the initial snapshot of the table, default is False. | -| `replay_lock` | `LockType` | The lock type used during replay, default is ‘shared’, can also be ‘exclusive’. | +| Parameter | Type | Description | +| ------------- | ------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `table` | `Table` | The table to listen to. | +| `listener` | `Callable[[TableUpdate, bool], None] \| TableListener` | Either a function or a [TableListener](https://deephaven.io/core/pydoc/code/deephaven.table_listener.html#deephaven.table_listener.TableListener) with an on_update function. The function must take a [TableUpdate](https://deephaven.io/core/pydoc/code/deephaven.table_listener.html#deephaven.table_listener.TableUpdate) and is_replay bool. [More table listener info](https://deephaven.io/core/docs/how-to-guides/table-listeners-python/) | +| `description` | `str \| None` | An optional description for the UpdatePerformanceTracker to append to the listener’s entry description, default is None. | +| `do_replay` | `bool` | Whether to replay the initial snapshot of the table, default is False. | +| `replay_lock` | `LockType` | The lock type used during replay, default is ‘shared’, can also be ‘exclusive’. | ##### use_table_data @@ -1637,7 +1637,6 @@ class LinkPoint(TypedDict): - #### Context By default, the context of a `@ui.component` will be created per client session (same as [Parameterized Query's "parallel universe" today](https://github.com/deephaven-ent/iris/blob/868b868fc9e180ee948137b10b6addbac043605e/ParameterizedQuery/src/main/java/io/deephaven/query/parameterized/impl/ParameterizedQueryServerImpl.java#L140)). However, it would be interesting if it were possible to share a context among all sessions for the current user, and/or share a context with other users even; e.g. if one user selects and applies a filter, it updates immediately for all other users with that dashboard open. So three cases: @@ -1772,15 +1771,15 @@ sequenceDiagram UIP->>SP: Render tft SP->>SP: Run sym_exchange Note over SP: sym_exchange executes, running text_filter_table twice - SP-->>UIP: Result (flex([tft1, tft2])) - UIP-->>W: Display (flex([tft1, tft2])) + SP-->>UIP: Result (document=flex([tft1, tft2]), exported_objects=[tft1, tft2]) + UIP-->>W: Display Result U->>UIP: Change text input 1 UIP->>SP: Change state SP->>SP: Run sym_exchange - Note over SP: sym_exchange executes, text_filter_table only
runs once for the one changed input - SP-->>UIP: Result (flex([tft1', tft2])) - UIP-->>W: Display (flex([tft1', tft2])) + Note over SP: sym_exchange executes, text_filter_table only
runs once for the one changed input
only exports the new table, as client already has previous tables + SP-->>UIP: Result (document=flex([tft1', tft2], exported_objects=[tft1'])) + UIP-->>W: Display Result ``` ##### Communication/Callbacks @@ -1807,12 +1806,11 @@ sequenceDiagram A component that is created on the server side runs through a few steps before it is rendered on the client side: -1. Element - The basis for all UI components. Generally a `FunctionElement`, and does not run the function until it is requested by the UI. The result can change depending on the context that it is rendered in (e.g. what "state" is set). -2. RenderedNode - After an element has been rendered using a renderer, it becomes a `RenderedNode`. This is an immutable representation of the document. -3. JSONEncodedNode - The `RenderedNode` is then encoded into JSON using `NodeEncoder`. It pulls out all the objects and maps them to exported objects, and all the callables to be mapped to commands that can be accepted by JSON-RPC. This is the final representation of the document that is sent to the client. -4. ElementPanel - Client side where it's receiving the `documentUpdated` from the server plugin, and then rendering the `JSONEncodedNode` into a `ElementPanel` (e.g. a `GoldenLayout` panel). Decodes the JSON, maps all the exported objects to the actual objects, and all the callables to async methods that will call to the server. -5. ElementView - Renders the decoded panel into the UI. Picks the element based on the name of it. -6. ObjectView - Render an exported object +1. [Element](./src/deephaven/ui/elements/Element.py) - The basis for all UI components. Generally a [FunctionElement](./src/deephaven/ui/elements/FunctionElement.py) created by a script using the [@ui.component](./src/deephaven/ui/components/make_component.py) decorator, and does not run the function until it is rendered. The result can change depending on the context that it is rendered in (e.g. what "state" is set). +2. [ElementMessageStream](./src/deephaven/ui/object_types/ElementMessageStream.py) - The `ElementMessageStream` is responsible for rendering one instance of an element in a specific rendering context and handling the server-client communication. The element is rendered to create a [RenderedNode](./src/deephaven/ui/renderer/RenderedNode.py), which is an immutable representation of a rendered document. The `RenderedNode` is then encoded into JSON using [NodeEncoder](./src/deephaven/ui/renderer/NodeEncoder.py), which pulls out all the non-serializable objects (such as Tables) and maps them to exported objects, and all the callables to be mapped to commands that can be accepted by JSON-RPC. This is the final representation of the document that is sent to the client, and ultimately handled by the `WidgetHandler`. +3. [DashboardPlugin](./src/js/src/DashboardPlugin.tsx) - Client side `DashboardPlugin` that listens for when a widget of type `Element` is opened, and manage the `WidgetHandler` instances that are created for each widget. +4. [WidgetHandler](./src/js/src/WidgetHandler.tsx) - Uses JSON-RPC communication with an `ElementMessageStream` instance to load the initial rendered document and associated exported objects. Listens for any changes and updates the document accordingly. +5. [DocumentHandler](./src/js/src/DocumentHandler.tsx) - Handles the root of a rendered document, laying out the appropriate panels or dashboard specified. #### Other Decisions diff --git a/plugins/ui/src/deephaven/ui/components/make_component.py b/plugins/ui/src/deephaven/ui/components/make_component.py index 326873686..b7e43c652 100644 --- a/plugins/ui/src/deephaven/ui/components/make_component.py +++ b/plugins/ui/src/deephaven/ui/components/make_component.py @@ -1,18 +1,23 @@ import functools import logging +from typing import Any, Callable from .._internal import get_component_qualname from ..elements import FunctionElement logger = logging.getLogger(__name__) -def make_component(func): +def make_component(func: Callable[..., Any]): """ Create a FunctionalElement from the passed in function. + + Args: + func: The function to create a FunctionalElement from. + Runs when the component is being rendered. """ @functools.wraps(func) - def make_component_node(*args, **kwargs): + def make_component_node(*args: Any, **kwargs: Any): component_type = get_component_qualname(func) return FunctionElement(component_type, lambda: func(*args, **kwargs)) diff --git a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py index 2cd23cc07..627bac70d 100644 --- a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py +++ b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py @@ -13,6 +13,36 @@ class ElementMessageStream(MessageStream): + _manager: JSONRPCResponseManager + """ + Handle incoming requests from the client. + """ + + _dispatcher: Dispatcher + """ + The dispatcher to use when client calls callables. + """ + + _encoder: NodeEncoder + """ + Encoder to use to encode the document. + """ + + _message_id: int + """ + The next message ID to use. + """ + + _element: Element + """ + The element to render. + """ + + _connection: MessageStream + """ + The connection to send the rendered element to. + """ + def __init__(self, element: Element, connection: MessageStream): """ Create a new ElementMessageStream. Renders the element in a render context, and sends the rendered result to the From 53020b4f14cbfdb8d4ba90d2929073aa5c3cff1a Mon Sep 17 00:00:00 2001 From: mikebender Date: Wed, 29 Nov 2023 13:33:33 -0500 Subject: [PATCH 05/11] WIP WidgetHandler proxying the fetch/close calls --- plugins/ui/src/js/src/UITable.tsx | 28 ++++++-- plugins/ui/src/js/src/WidgetHandler.tsx | 90 ++++++++++++++++++------- 2 files changed, 88 insertions(+), 30 deletions(-) diff --git a/plugins/ui/src/js/src/UITable.tsx b/plugins/ui/src/js/src/UITable.tsx index 7106650b8..fa5b31364 100644 --- a/plugins/ui/src/js/src/UITable.tsx +++ b/plugins/ui/src/js/src/UITable.tsx @@ -1,5 +1,9 @@ import React, { useEffect, useMemo, useState } from 'react'; -import { IrisGridProps } from '@deephaven/iris-grid'; +import { + IrisGridModel, + IrisGridModelFactory, + IrisGridProps, +} from '@deephaven/iris-grid'; import { useApi } from '@deephaven/jsapi-bootstrap'; import type { Table } from '@deephaven/jsapi-types'; import Log from '@deephaven/log'; @@ -14,24 +18,36 @@ export interface UITableProps { function UITable({ element }: UITableProps) { const dh = useApi(); - const [table, setTable] = useState(); + const [model, setModel] = useState(); const { props: elementProps } = element; // Just load the object on mount useEffect(() => { + let isCancelled = false; async function loadModel() { - log.debug('Loading table from props', element.props); - const newTable = (await element.props.table.fetch()) as Table; - setTable(newTable); + log.debug('Loading table from props', elementProps); + const newTable = (await elementProps.table.fetch()) as Table; + const newModel = await IrisGridModelFactory.makeModel(dh, newTable); + if (!isCancelled) { + setModel(newModel); + } else { + newModel.close(); + } } loadModel(); - }, [dh, element]); + return () => { + isCancelled = true; + }; + }, [dh, elementProps]); const irisGridProps: Partial = useMemo(() => { const { onRowDoublePress } = elementProps; return { onDataSelected: onRowDoublePress }; }, [elementProps]); + // We want to clean up the model when we unmount or get a new model + useEffect(() => () => model?.close(), [model]); + return table ? ( ) : null; diff --git a/plugins/ui/src/js/src/WidgetHandler.tsx b/plugins/ui/src/js/src/WidgetHandler.tsx index 421f4b2fe..f8a051f67 100644 --- a/plugins/ui/src/js/src/WidgetHandler.tsx +++ b/plugins/ui/src/js/src/WidgetHandler.tsx @@ -41,18 +41,23 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { const [widget, setWidget] = useState(); const [element, setElement] = useState(); - // We keep a weak reference to all the exported objects so they can be garbage collected when we no longer need them - const exportedObjectMap = useRef>>( + + // When we fetch a widget, the client is then responsible for the exported objects. + // These objects could stay alive even after the widget is closed if we wanted to, + // but for our use case we want to close them when the widget is closed, so we close them all on unmount. + const exportedObjectMap = useRef>( new Map() ); const exportedObjectCount = useRef(0); - // We keep a ref to all the exported objects in the current render so they are _not_ garbage collected - const liveObjects = useRef([]); useEffect( - () => () => { - widget?.close(); - }, + () => + function closeWidget() { + exportedObjectMap.current.forEach(exportedObject => { + exportedObject.close(); + }); + widget?.close(); + }, [widget] ); @@ -78,10 +83,50 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { * Element nodes are not replaced. Those are handled in `DocumentHandler`. * * @param data The data to parse + * @param newExportedObjects New exported objects to add to the map * @returns The parsed data */ - (data: string) => { - const newLiveObjects: WidgetExportedObject[] = []; + (data: string, newExportedObjects: WidgetExportedObject[]) => { + // Keep track of exported objects that are no longer in use after this render. + // We close those objects that are no longer referenced, as they will never be referenced again. + const deadObjectMap = new Map(exportedObjectMap.current); + const newExportedObjectMap = new Map(exportedObjectMap.current); + + for (let i = 0; i < newExportedObjects.length; i += 1) { + const exportedObject = newExportedObjects[i]; + const exportedObjectKey = exportedObjectCount.current; + exportedObjectCount.current += 1; + + if (exportedObject.type === 'Table') { + // Table has some special handling compared to other widgets + // We want to return a copy of the table, and only release the table object when the widget is actually closed + } + // Some elements may fetch the object, then be hidden and close the object temporarily, and then shown again. + // We can only fetch each exported object once, so just fetch it once and cache it, then subscribe/unsubscribe as needed. + const cachedObject: [Promise | undefined] = [undefined]; + const proxyObject = new Proxy(exportedObject, { + get: (target, prop, ...rest) => { + if (prop === 'fetch') { + return () => { + if (cachedObject[0] === undefined) { + cachedObject[0] = target.fetch(); + } + return cachedObject[0]; + }; + } + // if (prop === 'close') { + // return () => { + // // We only want to unsubscribe from the object here, not close it. We will close it when the widget is closed, but until then the object may be needed again. + // (cachedObject[0] as any).unsubscribe(); + // }; + // } + return Reflect.get(target, prop, ...rest); + }, + }); + + newExportedObjectMap.set(exportedObjectKey, proxyObject); + } + const parsedData = JSON.parse(data, (key, value) => { // Need to re-hydrate any objects that are defined if (isCallableNode(value)) { @@ -95,20 +140,26 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { if (isObjectNode(value)) { // Replace this node with the exported object const objectKey = value[OBJECT_KEY]; - const exportedObject = exportedObjectMap.current - .get(objectKey) - ?.deref(); + const exportedObject = newExportedObjectMap.get(objectKey); if (exportedObject === undefined) { // The map should always have the exported object for a key, otherwise the protocol is broken throw new Error(`Invalid exported object key ${objectKey}`); } - newLiveObjects.push(exportedObject); + deadObjectMap.delete(objectKey); return exportedObject; } return value; }); - liveObjects.current = newLiveObjects; + + // Close any objects that are no longer referenced + deadObjectMap.forEach((deadObject, objectKey) => { + log.debug('Closing dead object', objectKey); + deadObject.close(); + newExportedObjectMap.delete(objectKey); + }); + + exportedObjectMap.current = newExportedObjectMap; return parsedData; }, [jsonClient] @@ -142,16 +193,7 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { newExportedObjects: WidgetExportedObject[] ) { log.debug2('Data received', data, newExportedObjects); - for (let i = 0; i < newExportedObjects.length; i += 1) { - const exportedObject = newExportedObjects[i]; - const exportedObjectKey = exportedObjectCount.current; - exportedObjectCount.current += 1; - exportedObjectMap.current.set( - exportedObjectKey, - new WeakRef(exportedObject) - ); - } - const parsedData = parseData(data); + const parsedData = parseData(data, newExportedObjects); jsonClient?.receiveAndSend(parsedData); } From c5e8c3b3884f7c2409d07ff2f89948dd252353bf Mon Sep 17 00:00:00 2001 From: mikebender Date: Thu, 30 Nov 2023 11:20:58 -0500 Subject: [PATCH 06/11] WIP some debugging stuff --- plugins/ui/src/js/src/WidgetHandler.tsx | 30 +++++++++++++++++-------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/plugins/ui/src/js/src/WidgetHandler.tsx b/plugins/ui/src/js/src/WidgetHandler.tsx index f8a051f67..42ecc95d6 100644 --- a/plugins/ui/src/js/src/WidgetHandler.tsx +++ b/plugins/ui/src/js/src/WidgetHandler.tsx @@ -97,22 +97,34 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { const exportedObjectKey = exportedObjectCount.current; exportedObjectCount.current += 1; - if (exportedObject.type === 'Table') { - // Table has some special handling compared to other widgets - // We want to return a copy of the table, and only release the table object when the widget is actually closed - } + // if (exportedObject.type === 'Table') { + // // Table has some special handling compared to other widgets + // // We want to return a copy of the table, and only release the table object when the widget is actually closed + // } // Some elements may fetch the object, then be hidden and close the object temporarily, and then shown again. // We can only fetch each exported object once, so just fetch it once and cache it, then subscribe/unsubscribe as needed. const cachedObject: [Promise | undefined] = [undefined]; const proxyObject = new Proxy(exportedObject, { get: (target, prop, ...rest) => { if (prop === 'fetch') { - return () => { - if (cachedObject[0] === undefined) { - cachedObject[0] = target.fetch(); - } - return cachedObject[0]; + return async () => { + const newObject = await target.fetch(); + console.log('XXX newObj', newObject); + const newObj2 = await target.fetch(); + console.log('XXX newObj2', newObj2); + (newObject as any).close(); + console.log('XXX newObj closed'); + const newObj3 = await target.fetch(); + console.log('XXX newObj3', newObj3); + (newObj2 as any).close(); + return newObj3; }; + // return () => { + // if (cachedObject[0] === undefined) { + // cachedObject[0] = target.fetch(); + // } + // return cachedObject[0]; + // }; } // if (prop === 'close') { // return () => { From 1cc914c1ef13c27e0ed8a2f1d3f7f5b54a781354 Mon Sep 17 00:00:00 2001 From: mikebender Date: Mon, 4 Dec 2023 11:30:29 -0500 Subject: [PATCH 07/11] Changed `parseData` method to `parseDocument` - It cleans up dead objects, which we only want to do when parsing an actual documentUpdated request rather than decoding any response (such as a response for a callback) - Cleaned up how ElementMessageStream encodes the payload and sends it back. --- .../ui/object_types/ElementMessageStream.py | 13 ++- plugins/ui/src/js/src/UITable.tsx | 28 +---- plugins/ui/src/js/src/WidgetHandler.tsx | 110 +++++++----------- plugins/ui/src/js/src/WidgetTestUtils.ts | 2 +- 4 files changed, 53 insertions(+), 100 deletions(-) diff --git a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py index 627bac70d..014ffe756 100644 --- a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py +++ b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py @@ -1,5 +1,6 @@ from __future__ import annotations import io +import json from jsonrpc import JSONRPCResponseManager, Dispatcher import logging from typing import Any @@ -83,9 +84,9 @@ def on_data(self, payload: bytes, references: list[Any]) -> None: if response is None: return - payload = response.json - logger.debug("Response: %s, %s", type(payload), payload) - self._connection.on_data(payload.encode(), []) + response_payload = response.json + logger.debug("Response: %s, %s", type(response_payload), response_payload) + self._connection.on_data(response_payload.encode(), []) def _get_next_message_id(self) -> int: self._message_id += 1 @@ -129,9 +130,9 @@ def _send_document_update(self, root: RenderedNode) -> None: """ # TODO(#67): Send a diff of the document instead of the entire document. - request = self._make_notification("documentUpdated", root) - payload = self._encoder.encode(request) - + encoded_document = self._encoder.encode(root) + request = self._make_notification("documentUpdated", encoded_document) + payload = json.dumps(request) logger.debug(f"Sending payload: {payload}") dispatcher = Dispatcher() diff --git a/plugins/ui/src/js/src/UITable.tsx b/plugins/ui/src/js/src/UITable.tsx index fa5b31364..7106650b8 100644 --- a/plugins/ui/src/js/src/UITable.tsx +++ b/plugins/ui/src/js/src/UITable.tsx @@ -1,9 +1,5 @@ import React, { useEffect, useMemo, useState } from 'react'; -import { - IrisGridModel, - IrisGridModelFactory, - IrisGridProps, -} from '@deephaven/iris-grid'; +import { IrisGridProps } from '@deephaven/iris-grid'; import { useApi } from '@deephaven/jsapi-bootstrap'; import type { Table } from '@deephaven/jsapi-types'; import Log from '@deephaven/log'; @@ -18,36 +14,24 @@ export interface UITableProps { function UITable({ element }: UITableProps) { const dh = useApi(); - const [model, setModel] = useState(); + const [table, setTable] = useState
(); const { props: elementProps } = element; // Just load the object on mount useEffect(() => { - let isCancelled = false; async function loadModel() { - log.debug('Loading table from props', elementProps); - const newTable = (await elementProps.table.fetch()) as Table; - const newModel = await IrisGridModelFactory.makeModel(dh, newTable); - if (!isCancelled) { - setModel(newModel); - } else { - newModel.close(); - } + log.debug('Loading table from props', element.props); + const newTable = (await element.props.table.fetch()) as Table; + setTable(newTable); } loadModel(); - return () => { - isCancelled = true; - }; - }, [dh, elementProps]); + }, [dh, element]); const irisGridProps: Partial = useMemo(() => { const { onRowDoublePress } = elementProps; return { onDataSelected: onRowDoublePress }; }, [elementProps]); - // We want to clean up the model when we unmount or get a new model - useEffect(() => () => model?.close(), [model]); - return table ? ( ) : null; diff --git a/plugins/ui/src/js/src/WidgetHandler.tsx b/plugins/ui/src/js/src/WidgetHandler.tsx index 42ecc95d6..3d00b9442 100644 --- a/plugins/ui/src/js/src/WidgetHandler.tsx +++ b/plugins/ui/src/js/src/WidgetHandler.tsx @@ -50,17 +50,6 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { ); const exportedObjectCount = useRef(0); - useEffect( - () => - function closeWidget() { - exportedObjectMap.current.forEach(exportedObject => { - exportedObject.close(); - }); - widget?.close(); - }, - [widget] - ); - // Bi-directional communication as defined in https://www.npmjs.com/package/json-rpc-2.0 const jsonClient = useMemo( () => @@ -75,7 +64,7 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { : null, [widget] ); - const parseData = useCallback( + const parseDocument = useCallback( /** * Parse the data from the server, replacing any callable nodes with functions that call the server. * Replaces all Callables with an async callback that will automatically call the server use JSON-RPC. @@ -83,61 +72,12 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { * Element nodes are not replaced. Those are handled in `DocumentHandler`. * * @param data The data to parse - * @param newExportedObjects New exported objects to add to the map * @returns The parsed data */ - (data: string, newExportedObjects: WidgetExportedObject[]) => { + (data: string) => { // Keep track of exported objects that are no longer in use after this render. // We close those objects that are no longer referenced, as they will never be referenced again. const deadObjectMap = new Map(exportedObjectMap.current); - const newExportedObjectMap = new Map(exportedObjectMap.current); - - for (let i = 0; i < newExportedObjects.length; i += 1) { - const exportedObject = newExportedObjects[i]; - const exportedObjectKey = exportedObjectCount.current; - exportedObjectCount.current += 1; - - // if (exportedObject.type === 'Table') { - // // Table has some special handling compared to other widgets - // // We want to return a copy of the table, and only release the table object when the widget is actually closed - // } - // Some elements may fetch the object, then be hidden and close the object temporarily, and then shown again. - // We can only fetch each exported object once, so just fetch it once and cache it, then subscribe/unsubscribe as needed. - const cachedObject: [Promise | undefined] = [undefined]; - const proxyObject = new Proxy(exportedObject, { - get: (target, prop, ...rest) => { - if (prop === 'fetch') { - return async () => { - const newObject = await target.fetch(); - console.log('XXX newObj', newObject); - const newObj2 = await target.fetch(); - console.log('XXX newObj2', newObj2); - (newObject as any).close(); - console.log('XXX newObj closed'); - const newObj3 = await target.fetch(); - console.log('XXX newObj3', newObj3); - (newObj2 as any).close(); - return newObj3; - }; - // return () => { - // if (cachedObject[0] === undefined) { - // cachedObject[0] = target.fetch(); - // } - // return cachedObject[0]; - // }; - } - // if (prop === 'close') { - // return () => { - // // We only want to unsubscribe from the object here, not close it. We will close it when the widget is closed, but until then the object may be needed again. - // (cachedObject[0] as any).unsubscribe(); - // }; - // } - return Reflect.get(target, prop, ...rest); - }, - }); - - newExportedObjectMap.set(exportedObjectKey, proxyObject); - } const parsedData = JSON.parse(data, (key, value) => { // Need to re-hydrate any objects that are defined @@ -152,7 +92,7 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { if (isObjectNode(value)) { // Replace this node with the exported object const objectKey = value[OBJECT_KEY]; - const exportedObject = newExportedObjectMap.get(objectKey); + const exportedObject = exportedObjectMap.current.get(objectKey); if (exportedObject === undefined) { // The map should always have the exported object for a key, otherwise the protocol is broken throw new Error(`Invalid exported object key ${objectKey}`); @@ -168,15 +108,34 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { deadObjectMap.forEach((deadObject, objectKey) => { log.debug('Closing dead object', objectKey); deadObject.close(); - newExportedObjectMap.delete(objectKey); + exportedObjectMap.current.delete(objectKey); }); - exportedObjectMap.current = newExportedObjectMap; + log.debug2( + 'Parsed data', + parsedData, + 'exportedObjectMap', + exportedObjectMap.current, + 'deadObjectMap', + deadObjectMap + ); return parsedData; }, [jsonClient] ); + const updateExportedObjects = useCallback( + (newExportedObjects: WidgetExportedObject[]) => { + for (let i = 0; i < newExportedObjects.length; i += 1) { + const exportedObject = newExportedObjects[i]; + const exportedObjectKey = exportedObjectCount.current; + exportedObjectCount.current += 1; + exportedObjectMap.current.set(exportedObjectKey, exportedObject); + } + }, + [] + ); + useEffect( function initMethods() { if (jsonClient == null) { @@ -184,16 +143,17 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { } log.debug('Adding methods to jsonClient'); - jsonClient.addMethod('documentUpdated', async (params: [ElementNode]) => { + jsonClient.addMethod('documentUpdated', async (params: [string]) => { log.debug2('documentUpdated', params[0]); - setElement(params[0]); + const newDocument = parseDocument(params[0]); + setElement(newDocument); }); return () => { jsonClient.rejectAllPendingRequests('Widget was changed'); }; }, - [jsonClient] + [jsonClient, parseDocument] ); useEffect(() => { @@ -205,8 +165,8 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { newExportedObjects: WidgetExportedObject[] ) { log.debug2('Data received', data, newExportedObjects); - const parsedData = parseData(data, newExportedObjects); - jsonClient?.receiveAndSend(parsedData); + updateExportedObjects(newExportedObjects); + jsonClient?.receiveAndSend(JSON.parse(data)); } const cleanup = widget.addEventListener( @@ -227,7 +187,7 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { log.debug('Cleaning up listener'); cleanup(); }; - }, [dh, jsonClient, parseData, widget]); + }, [dh, jsonClient, parseDocument, updateExportedObjects, widget]); useEffect( function loadWidget() { @@ -250,6 +210,14 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { [wrapper] ); + useEffect( + () => + function closeWidget() { + widget?.close(); + }, + [widget] + ); + const handleDocumentClose = useCallback(() => { log.debug('Widget document closed', wrapper.id); onClose?.(wrapper.id); diff --git a/plugins/ui/src/js/src/WidgetTestUtils.ts b/plugins/ui/src/js/src/WidgetTestUtils.ts index 1db3930f0..f84f7a54f 100644 --- a/plugins/ui/src/js/src/WidgetTestUtils.ts +++ b/plugins/ui/src/js/src/WidgetTestUtils.ts @@ -9,7 +9,7 @@ export function makeDocumentUpdatedJsonRpc( return { jsonrpc: '2.0', method: 'documentUpdated', - params: [document], + params: [JSON.stringify(document)], }; } From 638f4d12a6c9e514b7c73383c6f20b1f6a34aed1 Mon Sep 17 00:00:00 2001 From: mikebender Date: Fri, 8 Dec 2023 12:55:15 -0500 Subject: [PATCH 08/11] Pass the `false` flag for takeOwnership - Based on latest review cleanup --- plugins/ui/src/js/src/WidgetHandler.tsx | 2 +- plugins/ui/src/js/src/WidgetTypes.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/ui/src/js/src/WidgetHandler.tsx b/plugins/ui/src/js/src/WidgetHandler.tsx index 3d00b9442..cb2283dd4 100644 --- a/plugins/ui/src/js/src/WidgetHandler.tsx +++ b/plugins/ui/src/js/src/WidgetHandler.tsx @@ -194,7 +194,7 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { log.debug('loadWidget', wrapper.id, wrapper.definition); let isCancelled = false; async function loadWidgetInternal() { - const newWidget = await wrapper.fetch(); + const newWidget = await wrapper.fetch(false); if (isCancelled) { newWidget.close(); return; diff --git a/plugins/ui/src/js/src/WidgetTypes.ts b/plugins/ui/src/js/src/WidgetTypes.ts index f0ca0ca3c..c4ed147a3 100644 --- a/plugins/ui/src/js/src/WidgetTypes.ts +++ b/plugins/ui/src/js/src/WidgetTypes.ts @@ -9,7 +9,7 @@ export interface WidgetMessageDetails { export type WidgetMessageEvent = CustomEvent; -export type WidgetFetch = () => Promise; +export type WidgetFetch = (takeOwnership?: boolean) => Promise; export type WidgetWrapper = { definition: WidgetDefinition; From d120b838b86a8aa2309bdfcab21f80dcc52ef9e3 Mon Sep 17 00:00:00 2001 From: mikebender Date: Mon, 11 Dec 2023 16:10:05 -0500 Subject: [PATCH 09/11] Use Colin's latest changes with a `reexport` and `fetch` --- plugins/ui/src/js/src/ObjectView.tsx | 6 +++++- plugins/ui/src/js/src/UITable.tsx | 10 +++++++++- plugins/ui/src/js/src/WidgetHandler.tsx | 25 +++++++++++++++---------- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/plugins/ui/src/js/src/ObjectView.tsx b/plugins/ui/src/js/src/ObjectView.tsx index 987068ef5..5a6f4eb95 100644 --- a/plugins/ui/src/js/src/ObjectView.tsx +++ b/plugins/ui/src/js/src/ObjectView.tsx @@ -13,7 +13,11 @@ function ObjectView(props: ObjectViewProps) { const { object } = props; log.info('Object is', object); - const fetch = useCallback(() => object.fetch() as Promise, [object]); + const fetch = useCallback(async () => { + // We reexport the object in case this object is used in multiplate places or closed/opened multiple times + const reexportedObject = await object.reexport(); + return reexportedObject.fetch() as Promise; + }, [object]); const plugins = usePlugins(); diff --git a/plugins/ui/src/js/src/UITable.tsx b/plugins/ui/src/js/src/UITable.tsx index 7106650b8..d1f971df5 100644 --- a/plugins/ui/src/js/src/UITable.tsx +++ b/plugins/ui/src/js/src/UITable.tsx @@ -19,12 +19,20 @@ function UITable({ element }: UITableProps) { // Just load the object on mount useEffect(() => { + let isCancelled = false; async function loadModel() { log.debug('Loading table from props', element.props); - const newTable = (await element.props.table.fetch()) as Table; + const reexportedTable = await element.props.table.reexport(); + const newTable = (await reexportedTable.fetch()) as Table; + if (isCancelled) { + newTable.close(); + } setTable(newTable); } loadModel(); + return () => { + isCancelled = true; + }; }, [dh, element]); const irisGridProps: Partial = useMemo(() => { diff --git a/plugins/ui/src/js/src/WidgetHandler.tsx b/plugins/ui/src/js/src/WidgetHandler.tsx index cb2283dd4..9f2df8dfb 100644 --- a/plugins/ui/src/js/src/WidgetHandler.tsx +++ b/plugins/ui/src/js/src/WidgetHandler.tsx @@ -160,6 +160,10 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { if (widget == null) { return; } + // Need to reset the exported object map and count + const widgetExportedObjectMap = new Map(); + exportedObjectMap.current = widgetExportedObjectMap; + exportedObjectCount.current = 0; function receiveData( data: string, newExportedObjects: WidgetExportedObject[] @@ -184,8 +188,14 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { receiveData(widget.getDataAsString(), widget.exportedObjects); return () => { - log.debug('Cleaning up listener'); + log.debug('Cleaning up widget', widget); cleanup(); + widget.close(); + + // Clean up any exported objects that haven't been closed yet + Array.from(widgetExportedObjectMap.values()).forEach(exportedObject => { + exportedObject.close(); + }); }; }, [dh, jsonClient, parseDocument, updateExportedObjects, widget]); @@ -194,9 +204,12 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { log.debug('loadWidget', wrapper.id, wrapper.definition); let isCancelled = false; async function loadWidgetInternal() { - const newWidget = await wrapper.fetch(false); + const newWidget = await wrapper.fetch(); if (isCancelled) { newWidget.close(); + newWidget.exportedObjects.forEach(exportedObject => { + exportedObject.close(); + }); return; } log.debug('newWidget', wrapper.id, wrapper.definition, newWidget); @@ -210,14 +223,6 @@ function WidgetHandler({ onClose, widget: wrapper }: WidgetHandlerProps) { [wrapper] ); - useEffect( - () => - function closeWidget() { - widget?.close(); - }, - [widget] - ); - const handleDocumentClose = useCallback(() => { log.debug('Widget document closed', wrapper.id); onClose?.(wrapper.id); From b48d2535e644740aaa038c68e55f2f08309c9038 Mon Sep 17 00:00:00 2001 From: mikebender Date: Tue, 12 Dec 2023 16:31:36 -0500 Subject: [PATCH 10/11] Clean up based on review - NodeEncoder now has a encode_node method, and throws if you try and call `.encode()` directly - Fixed up tests --- .../ui/object_types/ElementMessageStream.py | 10 +++- .../src/deephaven/ui/renderer/NodeEncoder.py | 59 +++++++++++++++---- plugins/ui/src/js/src/WidgetTestUtils.ts | 2 + plugins/ui/test/deephaven/ui/test_encoder.py | 25 ++++---- 4 files changed, 69 insertions(+), 27 deletions(-) diff --git a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py index 014ffe756..1c1887702 100644 --- a/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py +++ b/plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py @@ -130,14 +130,18 @@ def _send_document_update(self, root: RenderedNode) -> None: """ # TODO(#67): Send a diff of the document instead of the entire document. - encoded_document = self._encoder.encode(root) + 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"] + request = self._make_notification("documentUpdated", encoded_document) payload = json.dumps(request) logger.debug(f"Sending payload: {payload}") dispatcher = Dispatcher() - for callable, callable_id in self._encoder.callable_dict.items(): + for callable, callable_id in callable_id_dict.items(): logger.debug("Registering callable %s", callable_id) dispatcher[callable_id] = callable self._dispatcher = dispatcher - self._connection.on_data(payload.encode(), self._encoder.new_objects) + self._connection.on_data(payload.encode(), new_objects) diff --git a/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py b/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py index acdb12632..ea0064067 100644 --- a/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py +++ b/plugins/ui/src/deephaven/ui/renderer/NodeEncoder.py @@ -1,7 +1,7 @@ from __future__ import annotations import json -from typing import Any, Callable +from typing import Any, Callable, TypedDict from weakref import WeakKeyDictionary from .RenderedNode import RenderedNode @@ -11,13 +11,34 @@ DEFAULT_CALLABLE_ID_PREFIX = "cb" -# IDs for callables are prefixes with a string and then use the `id` of the callable itself +# IDs for callables are a string of a callable prefix set and an incrementing ID CallableId = str # IDs for objects is just an incrementing ID. We should only send new exported objects with each render ObjectId = int +class NodeEncoderResult(TypedDict): + """ + Result of encoding a node. Contains the encoded node, list of new objects, and callables dictionary. + """ + + encoded_node: str + """ + The encoded node. + """ + + new_objects: list[Any] + """ + The list of new objects. + """ + + callable_id_dict: WeakKeyDictionary[Callable[..., Any], CallableId] + """ + Dictionary from a callable to the ID assigned to the callable. + """ + + class NodeEncoder(json.JSONEncoder): """ Encode the node in JSON. Store any replaced objects and callables in their respective arrays. @@ -26,12 +47,17 @@ class NodeEncoder(json.JSONEncoder): - non-serializable objects in the tree are replaced wtih an object with property `OBJECT_KEY` set to the index in the objects array. """ + _callable_id_prefix: str + """ + Prefix to use for callable ids. + """ + _next_callable_id: int """ - Prefix to use for callable ids. Used to ensure callables used in stream are unique. + Incrementing ID that is used to assign IDs to callables. Needs to be prefixed with the `_callable_id_prefix` to get an ID. """ - _callable_dict: WeakKeyDictionary[Callable[..., Any], str] + _callable_dict: WeakKeyDictionary[Callable[..., Any], CallableId] """ Dictionary from a callable to the ID assigned to the callable. """ @@ -62,6 +88,7 @@ def __init__( Args: *args: Arguments to pass to the JSONEncoder constructor + callable_id_prefix: Prefix to use for callable ids. Used to ensure callables used in stream are unique. **kwargs: Args to pass to the JSONEncoder constructor """ super().__init__(*args, **kwargs) @@ -85,16 +112,24 @@ def default(self, o: Any): return self._convert_object(o) def encode(self, o: Any) -> str: - self._new_objects = [] - return super().encode(o) + # Raise an error - should call encode_node instead + raise NotImplementedError("Use encode_node instead") - @property - def callable_dict(self) -> WeakKeyDictionary[Callable[..., Any], str]: - return self._callable_dict + def encode_node(self, node: RenderedNode) -> NodeEncoderResult: + """ + Encode the document, and return the encoded document and the list of new objects. - @property - def new_objects(self) -> list[Any]: - return self._new_objects + Args: + o: The document to encode + """ + # Reset the new objects list - they will get set when encoding + self._new_objects = [] + encoded_node = super().encode(node) + return { + "encoded_node": encoded_node, + "new_objects": self._new_objects, + "callable_id_dict": self._callable_dict, + } def _convert_rendered_node(self, node: RenderedNode): result: dict[str, Any] = {ELEMENT_KEY: node.name} diff --git a/plugins/ui/src/js/src/WidgetTestUtils.ts b/plugins/ui/src/js/src/WidgetTestUtils.ts index f84f7a54f..ad11e9264 100644 --- a/plugins/ui/src/js/src/WidgetTestUtils.ts +++ b/plugins/ui/src/js/src/WidgetTestUtils.ts @@ -34,10 +34,12 @@ export function makeWidgetDefinition({ export function makeWidget({ addEventListener = jest.fn(() => jest.fn()), getDataAsString = () => makeDocumentUpdatedJsonRpcString(), + exportedObjects = [], }: Partial = {}): Widget { return TestUtils.createMockProxy({ addEventListener, getDataAsString, + exportedObjects, }); } diff --git a/plugins/ui/test/deephaven/ui/test_encoder.py b/plugins/ui/test/deephaven/ui/test_encoder.py index 48e2e18b2..2d06e4f51 100644 --- a/plugins/ui/test/deephaven/ui/test_encoder.py +++ b/plugins/ui/test/deephaven/ui/test_encoder.py @@ -30,17 +30,17 @@ def expect_result( from deephaven.ui.renderer import NodeEncoder encoder = NodeEncoder(callable_id_prefix=callable_id_prefix) - payload = encoder.encode(node) + result = encoder.encode_node(node) self.assertDictEqual( - json.loads(payload), expected_payload, "payloads don't match" + json.loads(result["encoded_node"]), expected_payload, "payloads don't match" ) self.assertListEqual( - list(encoder.callable_dict.keys()), + list(result["callable_id_dict"].keys()), expected_callables, "callables don't match", ) self.assertListEqual( - encoder.new_objects, expected_objects, "objects don't match" + result["new_objects"], expected_objects, "objects don't match" ) def test_empty_document(self): @@ -385,7 +385,8 @@ def test_delta_objects(self): }, ) - payload = encoder.encode(node) + result = encoder.encode_node(node) + expected_payload = { "__dhElemName": "test0", "props": { @@ -419,12 +420,12 @@ def test_delta_objects(self): } self.assertDictEqual( - json.loads(payload), expected_payload, "payloads don't match" + json.loads(result["encoded_node"]), expected_payload, "payloads don't match" ) self.assertListEqual( - list(encoder.callable_dict.keys()), cbs, "callables don't match" + list(result["callable_id_dict"].keys()), cbs, "callables don't match" ) - self.assertListEqual(encoder.new_objects, objs, "objects don't match") + self.assertListEqual(result["new_objects"], objs, "objects don't match") # Add some new objects and callables to the mix for next encoding delta_objs = [TestObject()] @@ -447,7 +448,7 @@ def test_delta_objects(self): }, ) - payload = encoder.encode(node) + result = encoder.encode_node(node) expected_payload = { "__dhElemName": "test0", "props": { @@ -481,14 +482,14 @@ def test_delta_objects(self): } self.assertDictEqual( - json.loads(payload), expected_payload, "payloads don't match" + json.loads(result["encoded_node"]), expected_payload, "payloads don't match" ) self.assertListEqual( - list(encoder.callable_dict.keys()), + list(result["callable_id_dict"].keys()), [cbs[0], cbs[2], cbs[3]], "callables don't match", ) - self.assertListEqual(encoder.new_objects, delta_objs, "objects don't match") + self.assertListEqual(result["new_objects"], delta_objs, "objects don't match") if __name__ == "__main__": From 4b72d40d80b48aaac49ba1cc7a98a1e04326474f Mon Sep 17 00:00:00 2001 From: mikebender Date: Tue, 12 Dec 2023 19:26:15 -0500 Subject: [PATCH 11/11] Commit suggestion from Matt --- plugins/ui/src/js/src/ObjectView.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/ui/src/js/src/ObjectView.tsx b/plugins/ui/src/js/src/ObjectView.tsx index 5a6f4eb95..108d42d68 100644 --- a/plugins/ui/src/js/src/ObjectView.tsx +++ b/plugins/ui/src/js/src/ObjectView.tsx @@ -14,7 +14,7 @@ function ObjectView(props: ObjectViewProps) { log.info('Object is', object); const fetch = useCallback(async () => { - // We reexport the object in case this object is used in multiplate places or closed/opened multiple times + // We re-export the object in case this object is used in multiple places or closed/opened multiple times const reexportedObject = await object.reexport(); return reexportedObject.fetch() as Promise; }, [object]);