From 4dbd2c08892cd1f0f3c22668db2505158804fa48 Mon Sep 17 00:00:00 2001 From: Aran-Fey Date: Mon, 3 Mar 2025 12:42:10 +0100 Subject: [PATCH] replace key_scan with global data structure --- rio/component_meta.py | 9 ++ rio/components/component.py | 4 + rio/data_models.py | 5 +- rio/global_state.py | 17 ++- rio/session.py | 228 +++++++++++-------------------- rio/testing.py | 4 + tests/test_layouting/conftest.py | 2 +- tests/test_refresh.py | 33 ++--- tests/test_url_patterns.py | 2 +- 9 files changed, 130 insertions(+), 174 deletions(-) diff --git a/rio/component_meta.py b/rio/component_meta.py index 5e33b695..69825ee4 100644 --- a/rio/component_meta.py +++ b/rio/component_meta.py @@ -176,6 +176,15 @@ class ComponentMeta(RioDataclassMeta): ) ) + # If the component has a `key`, register it + if component.key is not None: + if component.key in global_state.key_to_component: + raise RuntimeError( + f'Multiple components share the key "{component.key}": {global_state.key_to_component[component.key]} and {component}' + ) + + global_state.key_to_component[component.key] = component + # Store a weak reference to the component's creator if global_state.currently_building_component is None: component._weak_creator_ = lambda: None diff --git a/rio/components/component.py b/rio/components/component.py index 5685c895..8da824d0 100644 --- a/rio/components/component.py +++ b/rio/components/component.py @@ -21,6 +21,7 @@ __all__ = ["Component"] T = t.TypeVar("T") +Key = str | int # Using `metaclass=ComponentMeta` makes this an abstract class, but since @@ -207,6 +208,9 @@ class Component(abc.ABC, metaclass=ComponentMeta): """ _: dataclasses.KW_ONLY + + # Unfortunately we have to inline the `Key` type here because dataclasses + # will create constructor signatures where `Key` can't be resolved. key: str | int | None = internal_field(default=None, init=True) min_width: float = 0 diff --git a/rio/data_models.py b/rio/data_models.py index fbe5ae89..3e69cb59 100644 --- a/rio/data_models.py +++ b/rio/data_models.py @@ -18,10 +18,11 @@ class BuildData: build_result: rio.Component all_children_in_build_boundary: set[rio.Component] + key_to_component: dict[rio.components.component.Key, rio.Component] # Keep track of how often this component has been built. This is used by - # components to determine whether they are still part of their parent's current - # build output. + # components to determine whether they are still part of their parent's + # current build output. build_generation: int diff --git a/rio/global_state.py b/rio/global_state.py index 69775b8a..2641f648 100644 --- a/rio/global_state.py +++ b/rio/global_state.py @@ -4,6 +4,8 @@ from pathlib import Path import rio +from .components import component + # This boolean indicates whether the program was started via `rio run`. This # lets us display more useful error messages if the user tries to execute # something like `app.run_in_window()`, which conflicts with `rio run`. @@ -16,8 +18,9 @@ launched_via_rio_run: bool = False rio_run_app_module_path: Path | None = None -# Before a component is built, this value is set to that component. This allows -# newly created components to determine their creator, as well as session. +# Before a component is built, this variable is set to that component. This +# allows newly created components to determine their creator, as well as +# session. # # - `Component`: The component that is currently being built # - `None`: The build method of the app itself is currently being called @@ -31,6 +34,12 @@ currently_building_component: rio.Component | None = None currently_building_session: rio.Session | None = None -# This counter is increased each time a component is built. It can thus be used to -# uniquely identify the build generation of every component. +# Keeps track of components that have a `key`` (and were instantiated during this +# `build`). The reconciler needs to know about every component with a `key`, and +# this is the fastest way to do it. +key_to_component: dict[component.Key, rio.Component] = {} + + +# This counter is increased each time a component is built. It can thus be used +# to uniquely identify the build generation of every component. build_generation: int = 0 diff --git a/rio/session.py b/rio/session.py index 03918b0f..62ff508f 100644 --- a/rio/session.py +++ b/rio/session.py @@ -1253,6 +1253,8 @@ window.location.href = {json.dumps(str(active_page_url))}; global_state.currently_building_component = None global_state.currently_building_session = None + key_to_component = global_state.key_to_component + global_state.key_to_component = {} if component in self._dirty_components: raise RuntimeError( @@ -1273,6 +1275,7 @@ window.location.href = {json.dumps(str(active_page_url))}; component_data = component._build_data_ = BuildData( build_result, set(), # Set of all children - filled in below + key_to_component, 0, ) @@ -1291,7 +1294,9 @@ window.location.href = {json.dumps(str(active_page_url))}; # - Update the component data with the build output resulting from # the operations above else: - self._reconcile_tree(component_data, build_result) + self._reconcile_tree( + component_data, build_result, key_to_component + ) # Reconciliation can change the build result. Make sure nobody # uses `build_result` instead of `component_data.build_result` @@ -1519,11 +1524,15 @@ window.location.href = {json.dumps(str(active_page_url))}; self, old_build_data: BuildData, new_build: rio.Component, + new_key_to_component: dict[rio.components.component.Key, rio.Component], ) -> None: # Find all pairs of components which should be reconciled matched_pairs = list( self._find_components_for_reconciliation( - old_build_data.build_result, new_build + old_build_data.build_result, + new_build, + old_build_data.key_to_component, + new_key_to_component, ) ) @@ -1536,6 +1545,13 @@ window.location.href = {json.dumps(str(active_page_url))}; for old_component, new_component in matched_pairs } + # Update the key to component mapping. Take the keys from the new dict, + # but replace the components with the reconciled ones. + old_build_data.key_to_component = { + key: reconciled_components_new_to_old.get(component, component) + for key, component in new_key_to_component.items() + } + # Reconcile all matched pairs for ( new_component, @@ -1794,6 +1810,8 @@ window.location.href = {json.dumps(str(active_page_url))}; self, old_build: rio.Component, new_build: rio.Component, + old_key_to_component: dict[rio.components.component.Key, rio.Component], + new_key_to_component: dict[rio.components.component.Key, rio.Component], ) -> t.Iterable[tuple[rio.Component, rio.Component]]: """ Given two component trees, find pairs of components which can be @@ -1801,174 +1819,84 @@ window.location.href = {json.dumps(str(active_page_url))}; components are considered to be the same is up to the implementation and best-effort. - Returns an iterable over (old_component, new_component) pairs, as well - as a list of all components occurring in the new tree, which did not - have a match in the old tree. + Returns an iterable over (old_component, new_component) pairs. """ - old_components_by_key: dict[str | int, rio.Component] = {} - new_components_by_key: dict[str | int, rio.Component] = {} - matches_by_topology: list[tuple[rio.Component, rio.Component]] = [] + def can_pair_up( + old_component: rio.Component, new_component: rio.Component + ) -> bool: + # Components of different type are never a pair + if type(old_component) != type(new_component): + return False - # First scan all components for topological matches, and also keep track - # of each component by its key - def register_component_by_key( - components_by_key: dict[str | int, rio.Component], + # Components with different keys are never a pair + if old_component.key != new_component.key: + return False + + # Don't reconcile a component with itself, it won't end well + if old_component is new_component: + return False + + return True + + # Maintain a queue of (old_component, new_component) pairs that MAY + # represent the same component. If they match, they will be yielded as + # results, and their children will also be compared with each other. + queue: list[tuple[rio.Component, rio.Component]] = [ + (old_build, new_build) + ] + + # Add the components that have keys. Simply throw all potential pairs + # into the queue. + for old_key, old_component in old_key_to_component.items(): + try: + new_component = new_key_to_component[old_key] + except KeyError: + continue + + queue.append((old_component, new_component)) + + def _extract_child_components( component: rio.Component, - ) -> None: - if component.key is None: - return + attr_name: str, + ) -> list[rio.Component]: + attr = getattr(component, attr_name, None) - # It's possible that the same component is registered twice, once - # from a key_scan caused by a failed structural match, and once from - # recursing into a successful key match. - if ( - component.key in components_by_key - and components_by_key[component.key] is not component - ): - raise RuntimeError( - f'Multiple components share the key "{component.key}": {components_by_key[component.key]} and {component}' - ) + if isinstance(attr, rio.Component): + return [attr] - components_by_key[component.key] = component + if isinstance(attr, list): + return [ + item for item in attr if isinstance(item, rio.Component) + ] - def key_scan( - components_by_key: dict[str | int, rio.Component], - component: rio.Component, - include_self: bool = True, - ) -> None: - for child in component._iter_direct_and_indirect_child_containing_attributes_( - include_self=include_self, - recurse_into_high_level_components=True, - ): - register_component_by_key(components_by_key, child) + return [] - def chain_to_children( - old_component: rio.Component, - new_component: rio.Component, - ) -> None: - def _extract_components( - attr: object, - ) -> list[rio.Component]: - if isinstance(attr, rio.Component): - return [attr] + # Process the queue one by one. + while queue: + old_component, new_component = queue.pop(0) - if isinstance(attr, list): - attr = t.cast(list[object], attr) + if not can_pair_up(old_component, new_component): + continue - return [ - item for item in attr if isinstance(item, rio.Component) - ] + yield old_component, new_component - return [] - - # Iterate over the children, but make sure to preserve the topology. + # Compare the children, but make sure to preserve the topology. # Can't just use `iter_direct_children` here, since that would # discard topological information. for ( attr_name ) in inspection.get_child_component_containing_attribute_names( - type(new_component) + type(old_component) ): - old_value = getattr(old_component, attr_name, None) - new_value = getattr(new_component, attr_name, None) - - old_components = _extract_components(old_value) - new_components = _extract_components(new_value) - - # Chain to the children - common = min(len(old_components), len(new_components)) - for old_child, new_child in zip(old_components, new_components): - worker(old_child, new_child) - - for old_child in old_components[common:]: - key_scan( - old_components_by_key, old_child, include_self=True - ) - - for new_child in new_components[common:]: - key_scan( - new_components_by_key, new_child, include_self=True - ) - - def worker( - old_component: rio.Component, - new_component: rio.Component, - ) -> None: - # If a component was passed to a container, it is possible that the - # container returns the same instance of that component in multiple - # builds. This would reconcile a component with itself, which ends - # in disaster. - if old_component is new_component: - return - - # Register the component by key - register_component_by_key(old_components_by_key, old_component) - register_component_by_key(new_components_by_key, new_component) - - # If the components' types or keys don't match, stop looking for - # topological matches. Just keep track of the children's keys. - if ( - type(old_component) is not type(new_component) - or old_component.key != new_component.key - ): - key_scan( - old_components_by_key, old_component, include_self=False + old_children = _extract_child_components( + old_component, attr_name ) - key_scan( - new_components_by_key, new_component, include_self=False + new_children = _extract_child_components( + new_component, attr_name ) - return - # Key matches are handled elsewhere, so if the key is not `None`, do - # nothing. We'd just end up doing the same work twice. - if old_component.key is not None: - return - - matches_by_topology.append((old_component, new_component)) - chain_to_children(old_component, new_component) - - worker(old_build, new_build) - - # Find matches by key and reconcile their children. This can produce new - # key matches, so we do it in a loop. - new_key_matches: set[str | int] = ( - old_components_by_key.keys() & new_components_by_key.keys() - ) - all_key_matches = new_key_matches - - while new_key_matches: - for key in new_key_matches: - old_component = old_components_by_key[key] - new_component = new_components_by_key[key] - - # If a component was passed to a container, it is possible that - # the container returns the same instance of that component in - # multiple builds. This would reconcile a component with itself, - # which ends in disaster. - if old_component is new_component: - continue - - # If the components have different types, even the same key - # can't make them match - if type(old_component) is not type(new_component): - continue - - yield (old_component, new_component) - - # Recurse into these two components - chain_to_children(old_component, new_component) - - # If any new key matches were found, repeat the process - new_key_matches = ( - old_components_by_key.keys() - & new_components_by_key.keys() - all_key_matches - ) - all_key_matches.update(new_key_matches) - - # Yield topological matches - for old_component, new_component in matches_by_topology: - yield old_component, new_component + queue.extend(zip(old_children, new_children)) def _register_font(self, font: text_style.Font) -> str: # Fonts are different from other assets because they need to be diff --git a/rio/testing.py b/rio/testing.py index 0ea28423..d0b8d057 100644 --- a/rio/testing.py +++ b/rio/testing.py @@ -44,6 +44,10 @@ class TestClient: use_ordered_dirty_set: bool = False, ): ... + # Note about `use_ordered_dirty_set`: It's tempting to make it `True` per + # default so that the unit tests are deterministic, but there have been + # plenty of tests in the past that only failed *sometimes*, and without that + # randomness we wouldn't have found the bug at all. def __init__( # type: ignore self, app_or_build: rio.App | t.Callable[[], rio.Component] | None = None, diff --git a/tests/test_layouting/conftest.py b/tests/test_layouting/conftest.py index 719eb759..d3c6551b 100644 --- a/tests/test_layouting/conftest.py +++ b/tests/test_layouting/conftest.py @@ -13,4 +13,4 @@ async def manage_server(): await cleanup() -pytestmark = pytest.mark.async_timeout(10) +pytestmark = pytest.mark.async_timeout(15) diff --git a/tests/test_refresh.py b/tests/test_refresh.py index f40dee38..af9f2b30 100644 --- a/tests/test_refresh.py +++ b/tests/test_refresh.py @@ -28,11 +28,15 @@ async def test_refresh_with_clean_root_component() -> None: async def test_rebuild_component_with_dead_parent() -> None: - class RootComponent(rio.Component): - content: rio.Component + class ChildUnmounter(rio.Component): + child: rio.Component + child_is_mounted: bool = True def build(self) -> rio.Component: - return self.content + if self.child_is_mounted: + return self.child + else: + return rio.Spacer() class ComponentWithState(rio.Component): state: str @@ -41,27 +45,24 @@ async def test_rebuild_component_with_dead_parent() -> None: return rio.Text(self.state) def build() -> rio.Component: - return RootComponent( - rio.Row( - ComponentWithState("Hello"), - rio.ProgressCircle(), - ) - ) + return ChildUnmounter(ComponentWithState("Hello")) - async with rio.testing.TestClient(build) as test_client: - # Change the component's state, but also remove its parent from the - # component tree - root_component = test_client.get_component(RootComponent) + async with rio.testing.TestClient( + build, use_ordered_dirty_set=True + ) as test_client: + # Change the component's state, but also remove it from the component + # tree + unmounter = test_client.get_component(ChildUnmounter) component = test_client.get_component(ComponentWithState) - progress_component = test_client.get_component(rio.ProgressCircle) component.state = "Hi" - root_component.content = progress_component + unmounter.child_is_mounted = False await test_client.refresh() # Make sure no data for dead components was sent to JS - assert test_client._last_updated_components == {root_component} + assert unmounter in test_client._last_updated_components + assert component not in test_client._last_updated_components async def test_unmount_and_remount() -> None: diff --git a/tests/test_url_patterns.py b/tests/test_url_patterns.py index 36611ef1..70f44ac2 100644 --- a/tests/test_url_patterns.py +++ b/tests/test_url_patterns.py @@ -540,7 +540,7 @@ def test_layout_parameters_arent_url_parameters() -> None: "min_width": "5", "min_height": "5", "margin": "5", - "_id": "5", + "_id_": "5", }, )