feat(ui): Optimize render cycle to only re-render components with state changes#1295
feat(ui): Optimize render cycle to only re-render components with state changes#1295mofojed wants to merge 12 commits intodeephaven:mainfrom
Conversation
…s and emphasize use_memo for expensive calculations
- Add _is_dirty and _has_dirty_descendant flags to RenderContext - Add _parent_context to track parent-child relationships - Implement _mark_dirty() to propagate dirty state to ancestors - Add tests for dirty tracking behavior
Add cached rendered output and selective re-rendering logic to avoid re-rendering clean components: RenderContext changes: - Add _cached_rendered_node and _cached_props fields - Store rendered output after each render Renderer changes: - Import FunctionElement to distinguish stateful components - Modify _render_element to check dirty state before re-rendering - Only FunctionElements (user-defined @ui.component) use caching - BaseElements (built-in ui.*, like ui.text) always re-render - Add _render_children_only to traverse clean subtrees - Add _render_props_without_opening_context helper functions Behavior changes: - Clean components return cached rendered output - Effects with no dependencies only run when component is dirty - Parent effects preserved when only children re-render All 120 tests pass.
Fixed a bug where subsequent state updates on child components would not trigger re-renders after the first update. Root cause: _mark_dirty() optimizes by stopping propagation when a parent already has _has_dirty_descendant=True. However, _render_list and _render_dict weren't clearing this flag after processing children, leaving stale flags that caused _mark_dirty() to stop early. Fix: Clear _has_dirty_descendant at the end of _render_list and _render_dict to ensure proper propagation on subsequent state changes. Added test_multiple_child_state_updates to verify multiple clicks on child components trigger the correct re-renders.
Added tests for key-based reconciliation (Phase 4.2): - test_different_keys_have_separate_state: verifies different keys maintain separate state - test_changed_key_creates_new_context: verifies changing keys creates fresh contexts - test_unused_key_context_is_unmounted: verifies unused contexts get unmounted Added tests for effect behavior with selective re-rendering (Phase 4.3): - test_clean_component_effects_dont_run: verifies effects don't run when component is clean but has dirty descendants - test_key_change_unmounts_old_component: verifies key changes properly unmount/remount components Updated plan document to mark Phase 4 as completed.
Added comprehensive benchmark tests in test_benchmark.py: - test_large_tree_leaf_state_change: 111 components, achieves 0.90% re-render rate - test_multiple_leaves_state_change: verifies multiple dirty leaves re-render correctly - test_render_timing: measures 38-44x speedup for selective vs full re-render - test_deep_nesting_performance: 20-level deep nesting, only leaf re-renders - test_sibling_isolation: verifies siblings don't re-render - test_parent_state_change_rerenders_all: documents parent state change behavior - test_parent_state_passed_to_children: documents current limitation Key findings: - Selective re-render is 38-44x faster than full re-render - <10% re-render target exceeded (0.90% achieved) - Current limitation: caching is context-dirty-state based, not props-based Updated plan document with benchmark results.
When using map() or generators as children (e.g., ui.view(map(...))), the lazy iterator would get exhausted after the first render. During selective re-rendering, the cached BaseElement would return its _props containing the exhausted iterator, resulting in empty children. Changes: - Add materialize_lazy_iterators() utility function that recursively converts map/generator objects to lists - Apply materialization in BaseElement.__init__ for both positional children and children passed via kwargs - Apply materialization in Renderer when caching FunctionElement props - Add unit tests for the utility function - Add regression test for keyed list with child state changes
There was a problem hiding this comment.
Pull request overview
This PR implements a selective re-rendering optimization for the deephaven.ui rendering system. Previously, when any component's state changed, the entire component tree would re-render. This optimization introduces dirty tracking so that only components with state changes (and their descendants) re-render, significantly improving performance.
Changes:
- Added dirty tracking infrastructure to RenderContext (_is_dirty, _has_dirty_descendant flags)
- Implemented render output caching to reuse clean component renders
- Added materialize_lazy_iterators utility to handle lazy iterators in props
- Comprehensive test coverage including unit tests, integration tests, and performance benchmarks
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/ui/src/deephaven/ui/_internal/RenderContext.py | Added dirty tracking flags, parent context tracking, and _mark_dirty propagation logic |
| plugins/ui/src/deephaven/ui/renderer/Renderer.py | Implemented selective re-rendering with caching, _render_children_only for clean parents with dirty descendants |
| plugins/ui/src/deephaven/ui/_internal/utils.py | Added materialize_lazy_iterators function to convert map/generators to lists |
| plugins/ui/src/deephaven/ui/elements/BaseElement.py | Applied materialize_lazy_iterators to children props to prevent iterator exhaustion |
| plugins/ui/src/deephaven/ui/_internal/init.py | Exported materialize_lazy_iterators utility function |
| plugins/ui/test/deephaven/ui/test_renderer.py | Added comprehensive tests for selective rendering including edge cases |
| plugins/ui/test/deephaven/ui/test_render.py | Added tests for dirty tracking propagation and lifecycle |
| plugins/ui/test/deephaven/ui/test_utils.py | Added tests for materialize_lazy_iterators with map and generators |
| plugins/ui/test/deephaven/ui/test_benchmark.py | New file with performance benchmarks validating optimization goals |
| plugins/ui/docs/add-interactivity/render-cycle.md | Updated documentation to explain selective re-rendering behavior |
| plans/optimize-render-cycle.md | New comprehensive implementation plan and design documentation |
| AGENTS.md | New developer guidelines for Python environment and testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if isinstance(item, (map, GeneratorType)): | ||
| # Materialize and recursively process contents | ||
| return [materialize_lazy_iterators(x) for x in item] |
There was a problem hiding this comment.
The materialize_lazy_iterators function only handles map and GeneratorType, but there are other lazy iterator types that could cause the same exhaustion issues (filter, zip, itertools objects like chain, cycle, etc.). If users pass these as props, they won't be materialized and could be exhausted on subsequent renders, leading to missing children or unexpected behavior.
Consider expanding the check to handle other common lazy iterator types, or use a more general approach like checking for collections.abc.Iterator (excluding strings and bytes).
There was a problem hiding this comment.
I wish we could just do a loop grabbing next(x) but I know we also had issues with sets in the past and would have issues with strings and bytes as Copilot pointed out. We could have a larger allowed list of Iterables in general though.
It would be nice if we had some sort of unsafe registration for custom Iterables so that we don't have to make any attempt at being exhaustive (which we couldn't be anyways), but this is getting into the weeds and would be a different ticket.
At the very least, don't know why we couldn't combine the first two checks here into one isinstance check
| # Render children without opening parent context | ||
| # This preserves parent's effects and state while updating children | ||
| if cached_props is not None: | ||
| rendered_props = _render_dict_in_open_context(cached_props, context) | ||
| else: | ||
| rendered_props = {} |
There was a problem hiding this comment.
There's a potential issue with child context lifecycle management in _render_children_only. When this function calls _render_dict_in_open_context without opening the parent context, it will traverse the props and call parent_context.get_child_context(). This appends keys to parent._collected_contexts, but since the parent context is never opened here, the lifecycle management in RenderContext.open() (which clears _collected_contexts and unmounts unused child contexts) won't run properly. This could lead to child contexts not being properly tracked or unmounted, potentially causing memory leaks or stale state issues.
Consider wrapping the _render_dict_in_open_context call in a minimal context management that handles _collected_contexts properly, or restructure to ensure proper lifecycle management of child contexts.
There was a problem hiding this comment.
This is true and could cause some annoying to debug issues. I think we just need to clear the collected_contexts afterwards; these contexts shouldn't be changing here because the state hasn't changed. Hmm.
There was a problem hiding this comment.
We also aren't re-rendering if the input props have changed, which is significant.
| if isinstance(item, (map, GeneratorType)): | ||
| # Materialize and recursively process contents | ||
| return [materialize_lazy_iterators(x) for x in item] |
There was a problem hiding this comment.
I wish we could just do a loop grabbing next(x) but I know we also had issues with sets in the past and would have issues with strings and bytes as Copilot pointed out. We could have a larger allowed list of Iterables in general though.
It would be nice if we had some sort of unsafe registration for custom Iterables so that we don't have to make any attempt at being exhaustive (which we couldn't be anyways), but this is getting into the weeds and would be a different ticket.
At the very least, don't know why we couldn't combine the first two checks here into one isinstance check
| @@ -0,0 +1,490 @@ | |||
| """ | |||
There was a problem hiding this comment.
I really like the idea of benchmarks but I wonder if it should be it's own thing rather than under tests.
I think this is fine for now, but if we were ever to consider more benchmarking a separate entry point would be great. Maybe something like the code coverage tool where we could see if our changes slowed down the code automatically.
|
Found a couple issues in this approach when I was digging in/testing that I'll need to think about a bit more. I'm going to close this one and do component memoization first: #1296 |
2 participants
ui.memoor a param on aui.componentto memoize a component (e.g. cache it unless props or state have changed