Skip to content

feat: add efficient single-item add/remove path for ListDataView#24217

Draft
mshabarov wants to merge 1 commit intomainfrom
add-single-item
Draft

feat: add efficient single-item add/remove path for ListDataView#24217
mshabarov wants to merge 1 commit intomainfrom
add-single-item

Conversation

@mshabarov
Copy link
Copy Markdown
Contributor

This PR is a first draft for the API requested in #22355 and serves a hint purpose rather than a complete solution.

Introduces ItemAddedEvent and ItemRemovedEvent on DataChangeEvent and new notifyItemAdded / notifyItemRemoved methods on DataProvider so a single-item change can be dispatched without triggering the full dataGenerator.destroyAllData() that refreshAll causes. DataCommunicator handles the new events by refreshing the viewport with a size recheck and, for removals, dropping just the removed item's key and per-item state.

AbstractListDataView's single-item methods (addItem, removeItem, addItemAfter, addItemBefore) now use this path so existing API stays unchanged but updates are no longer O(visible items) in server-side state churn. Default methods on DataProvider fall back to refreshAll so third-party providers keep working.

Introduces ItemAddedEvent and ItemRemovedEvent on DataChangeEvent and
new notifyItemAdded / notifyItemRemoved methods on DataProvider so a
single-item change can be dispatched without triggering the full
dataGenerator.destroyAllData() that refreshAll causes. DataCommunicator
handles the new events by refreshing the viewport with a size recheck
and, for removals, dropping just the removed item's key and per-item
state. AbstractListDataView's single-item methods (addItem, removeItem,
addItemAfter, addItemBefore) now use this path so existing API stays
unchanged but updates are no longer O(visible items) in server-side
state churn. Default methods on DataProvider fall back to refreshAll so
third-party providers keep working.

Fixes #22355

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown

Test Results

 1 394 files  ±0   1 394 suites  ±0   1h 15m 10s ⏱️ - 2m 3s
10 072 tests +7  10 002 ✅ +7  70 💤 ±0  0 ❌ ±0 
10 547 runs  +7  10 468 ✅ +7  79 💤 ±0  0 ❌ ±0 

Results for commit 5919ad8. ± Comparison against base commit 8450563.

@mshabarov
Copy link
Copy Markdown
Contributor Author

What else I'd expect in the API is a "add/remove single item" callback for the lazy loading case, where one can persist/delete items to/from a backend.

@SavageCZ
Copy link
Copy Markdown

I agree that the lazy loading/backend case may need a separate API or callback.

From our testing, the most important part for our current use case is the in-memory ListDataView path. We use a Grid as a live migration log, where a background task progressively reports about 50k result objects. These results are passed through a queue between the background thread and the UI thread, and then appended to the Grid inside UI.access().

We compared two approaches:

  1. Manually adding items to the backing list and calling refreshAll() at most once per UI update.
  2. Using GridListDataView.addItem(...).

The first approach produced around 34k UI tasks / refreshAll() calls for 50k results, but the UI progress was relatively smooth.

The second approach reduced the number of UI tasks to around 1.5k, but addItem(...) was still called about 50k times. With the current implementation, this performed worse: the UI sometimes stopped updating after around 1–2k displayed results, froze for several seconds, and then all remaining items appeared correctly at once.

My understanding is that this happens because addItem(...) still appears to go through refreshAll() internally for each added item.

So for our case, this PR looks like exactly the missing piece: keeping the existing addItem(...) API, but making it notify the Grid about a single added item instead of going through the full refreshAll() path.

The backend/lazy loading callback also sounds useful, but for our current scenario the in-memory addItem(...) improvement would already solve the main issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants