Track array-predicate model dependencies in O(1) via track_any_change - #12806
Open
tilladam wants to merge 3 commits into
Open
Track array-predicate model dependencies in O(1) via track_any_change#12806tilladam wants to merge 3 commits into
tilladam wants to merge 3 commits into
Conversation
Sibling to the array.any/array.all predicates (slint-ui#11989): returns the index of the first element for which the closure holds, or -1 if none match. Mirrors any/all end-to-end — parser/lookup, resolving, Rust and C++ codegen, api/cpp/include/private/slint_models.h, the interpreter, and core::model::model_find_index — reusing the existing Expression::Closure/ Type::Closure plumbing. Same experimental gate as any/all (tracked by slint-ui#12777); only usable inline, not stored or passed around, for the same reasons any/all are restricted that way. The interpreter reuses core's model_any/model_all/model_find_index for the row iteration and dependency tracking, rather than open-coding the loop a second time; a small eval_array_row_predicate helper does the per-row work shared by any/all/find-index (bind arg_name, evaluate the closure expression, restore the shadowed local var). Added as groundwork for a real fix to slint-ui#11970 (ComboBox/RadioGroup current-value writes), which needs a value-to-index reverse lookup that plain any/all can't express. changelog: Added an experimental `array.find-index((name) => condition)` predicate, returning the index of the first matching element or -1.
Follow-up to the review of slint-ui#12790: model_any/model_all/model_find_index used to call track_row_data_changes for every visited row, which inserts each row into the sorted tracked_rows vector — O(n log n) lookups plus O(n²) worst-case insertion to track a whole model, per binding evaluation. Add ModelTracker::track_any_change(row_count), registering the currently evaluated binding as a dependency of any change to the model: the row count or any row's data. The default implementation falls back to track_row_count_changes plus per-row track_row_data_changes — exactly the previous behavior, so forwarding or third-party trackers stay correct; the row_count parameter exists to make that fallback possible. ModelNotify overrides it with a single all_rows_tracked flag plus the two existing dirty-property reads: row_changed short-circuits the tracked_rows binary search on the flag, and the flag is cleared wherever tracked_rows is cleared (row_added/row_removed/reset), after which the dirtied binding re-registers on its next evaluation. The C++ Model class gets the identical treatment (all_rows_tracked member and a track_any_change function), and the C++ model_any/model_all/ model_find_index helpers use it instead of per-row row_data_tracked. One deliberate trade-off: per-row tracking was precise under short-circuiting — any/find-index stop at the first match, so later rows were untracked and changes to them didn't re-evaluate. track_any_change over-approximates: a change to any row re-evaluates the predicate, trading occasional spurious (cheap) re-evaluations for tracking cost independent of the model size. Also drops the T: Default bound from model_any/model_all, matching model_find_index; absent rows are now skipped rather than fed to the predicate as default values. changelog: Reduced the dependency-tracking cost of the array predicates (`array.any`/`array.all`/`array.find-index`) from per-row to constant.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the review discussion in #12790 (#12790 (comment)):
model_any/model_all/model_find_indexused to calltrack_row_data_changesfor every visited row, which inserts each row into the sortedtracked_rowsvector — O(n log n) lookups plus O(n²) worst-case insertion to track a whole model, per binding evaluation.Stacked on #12790 — please review only the last commit.
This adds
ModelTracker::track_any_change(row_count), registering the currently evaluated binding as a dependency of any change to the model (row count or any row's data):track_row_count_changes+ per-rowtrack_row_data_changes— exactly the previous behavior, so forwarding/third-party trackers stay correct. Therow_countparameter exists to make that correct fallback possible; happy to bikeshed the signature.ModelNotifyoverrides it with a singleall_rows_trackedflag plus the two existing dirty-property reads.row_changedshort-circuits thetracked_rowsbinary search on the flag; the flag is cleared wherevertracked_rowsis cleared (row_added/row_removed/reset), after which the dirtied binding re-registers on its next evaluation.Modelclass gets the identical treatment, and the C++model_any/model_all/model_find_indexhelpers use it instead of per-rowrow_data_tracked.One deliberate trade-off: per-row tracking was precise under short-circuiting —
any/find-indexstop at the first match, so later rows were untracked and changes to them didn't re-evaluate.track_any_changeover-approximates: a change to any row re-evaluates the predicate, trading occasional spurious (cheap) re-evaluations for tracking cost independent of the model size.Also drops the
T: Defaultbound frommodel_any/model_all, matchingmodel_find_indexafter the review fixes in #12790.Covered by a new Rust unit test (
test_any_change_tracking) and a C++ catch2 test ("Model any-change tracking") exercising re-evaluation on row changes past the match point, add/remove, and reset.