Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tables that have not changed between re-renders flicker #128

Closed
Tracked by #50
mofojed opened this issue Nov 21, 2023 · 0 comments · Fixed by #129
Closed
Tracked by #50

Tables that have not changed between re-renders flicker #128

mofojed opened this issue Nov 21, 2023 · 0 comments · Fixed by #129
Assignees
Milestone

Comments

@mofojed
Copy link
Member

mofojed commented Nov 21, 2023

When you update the state of a component, tables that should not be affected by the change "flicker".

You can see this problem manifest following these steps:
Steps to Reproduce:

  1. Create a widget with a table that remains the same across re-renders and another table that changes across re-renders, e.g.:
import deephaven.ui as ui
from deephaven.plot.express import data

stocks = data.stocks()

@ui.component
def my_widget(source):
    value, set_value = ui.use_state("CAT")

    return [
        ui.panel(ui.text_field(value=value, on_change=set_value, label="sym"), title="Sym Input"),
        ui.panel(source, title="Source"),
        ui.panel(source.where(f"sym=`{value}`"), title="Filtered")
    ]

mw2 = my_widget(stocks)
  1. Make changes in the text-field to update the Filtered table

Expected Results
2. Only the Filtered table should blink/update

Actual Results
2. The table that is not being changed flickers when changes are made

Right now with every change, we are re-sending the full document (#123) and re-sending all exported objects. This causes tables that have not been changed to flicker. We do not need to send items we have already sent.

@mofojed mofojed mentioned this issue Nov 21, 2023
89 tasks
@mofojed mofojed self-assigned this Nov 21, 2023
@mofojed mofojed added this to the November 2023 milestone Nov 21, 2023
@mofojed mofojed changed the title Do not re-export objects that do not change between re-renders Tables that have not changed between re-renders flicker Nov 21, 2023
mofojed added a commit that referenced this issue Dec 13, 2023
- Instead of resending the full list of exported objects each time, just
send the newly exported objects
- Increment the ID for each new object that is created
- Keep a weak reference to old objects so they are released when no
longer referenced
- Objects remain the same between renders 
- Tested using the steps in #128 
- Fixes #128, fixes #67
- Needs deephaven/deephaven-core#4909 to handle
fetching objects multiple times if necessary

BREAKING CHANGE: JSON protocol now keeps the IDs of objects and
callables stable between re-renders, and only exports new objects with
each documentUpdated event. The client must track the old objects
independently.
mofojed added a commit to mofojed/deephaven-plugins that referenced this issue Feb 7, 2024
- Referenced a table `t` that did not exist, corrected to reference `formatted_table`
- Needed to refactor how exported objects were being handled - server was just using a WeakKeyDictionary, but the objects were not guaranteed to get cleared out on the server and were being reused between re-renders. The client would always dump old objects after a render, so just always clear out the objects on the server from the previous render. Note that the WeakKeyDictionary is still safe for callables; the client just references them by ID and does not hold a reference to an object, so they can be cleared by the server GC.
- Fixes deephaven#265
- Tested using the Stock Rollup example fixed in this PR, it now works correctly.
- Also tested with snippet from deephaven#128, ensuring the table still does not flicker.
- Added another test to test more than one iteration
mofojed added a commit that referenced this issue Feb 8, 2024
- Referenced a table `t` that did not exist, corrected to reference
`formatted_table`
- Needed to refactor how exported objects were being handled - server
was just using a WeakKeyDictionary, but the objects were not guaranteed
to get cleared out on the server and were being reused between
re-renders. The client would always dump old objects after a render, so
just always clear out the objects on the server from the previous
render. Note that the WeakKeyDictionary is still safe for callables; the
client just references them by ID and does not hold a reference to an
object, so they can be cleared by the server GC.
- Fixes #265
- Tested using the Stock Rollup example fixed in this PR, it now works
correctly.
- Also tested with snippet from #128, ensuring the table still does not
flicker.
- Added another test to test more than one iteration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant