perf: Drop query cache entries lazily instead of searching on every removal - #3985
Draft
spydon wants to merge 2 commits into
Draft
perf: Drop query cache entries lazily instead of searching on every removal#3985spydon wants to merge 2 commits into
spydon wants to merge 2 commits into
Conversation
The suite measured a single 'two registered queries' churn case, which could not tell apart the cost of having one more cache from the cost of having a cache that matches most of the container. It also had no read-side measurement to weigh those costs against. Add churn variants for no caches, one cache, and a second cache that matches either 1 in 50 or 4 in 5 of the children, plus a read suite that compares a cached query against the whereType scan that an unregistered type falls back to.
…emoval Removing a component searched every matching query cache for it with List.remove, a linear scan plus a shift, so a removal cost O(n) in the size of each cache it matched, while the removal from the backing array itself is O(1). A cache that matched most of the container therefore made removals about three times more expensive. Mirror what the backing array already does: a removal only marks the caches it matches, and the entries of the removed components are dropped in a single pass, before anything reads, reorders or adds to the caches again. The pass covers any number of removals at once, so a removal is O(1) regardless of how many types are registered or how much of the container they match.
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
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.
Description
Stacked on top of #3960, which it targets as its base branch.
This started as a question about whether
strictModestill earns its place now thatComponentListis a flat array, and the benchmark written to answer it turned up anunrelated cost in the same code.
The measurement
benchmark/type_query_benchmark.dartmeasured a single "two registered queries" churn case,which cannot separate the cost of having one more cache from the cost of having a cache that
matches most of the container, and it had no read-side measurement to weigh those costs
against. It now runs four churn variants (no caches, one cache, and a second cache matching
either 1 in 50 or 4 in 5 of the children) and a read suite comparing a cached
query<T>()against the
whereType<T>()scan an unregistered type falls back to.The read numbers say the caches are still clearly worth having on top of the flat array: a
cached query is 12x faster than the scan at 1000 children, and still 4x faster at the 16
children a query such as
GestureHitboxes.hitboxestypically runs over. So the answer to theoriginal question is yes,
strictModestill guards something real, andfalseremains theright default.
The fix
The churn numbers showed something else. Adding a second cache that almost never matches was
free (1835 -> 1930 us), but adding one that matches four fifths of the container cost 3x
(1835 -> 5670 us). That is not the cost of having a cache:
removesearched every matchingcache for the removed component with
List.remove, a linear scan plus a shift, so a removalwas O(n) in the size of each cache it matched, while the removal from the backing array
itself is O(1).
This makes the caches follow what the backing array already does. A removal only marks the
caches it matches, and the entries of the removed components are dropped in a single pass
before anything reads, reorders or adds to the caches again. One pass covers any number of
removals, so a removal is O(1) regardless of how many types are registered or how much of the
container they match.
The one observable difference is for a caller that holds on to the
Iterablereturned by aprevious
query<T>()call across a removal: it now keeps seeing the removed component untilthe next compaction, where before it would have been mutated underneath the caller (throwing
ConcurrentModificationErrorif that caller was mid-iteration). Every fresh read compactsfirst, so any new
query<T>()orwhereType<T>()call is always up to date.Benchmark results
Full
benchmark/main.dart, before and after, on the same machine (flutter test, JIT):The remaining suites (traversal, lifecycle churn, priority changes, hit testing, collision
detection, render) are unchanged within run-to-run noise, as expected: nothing outside the
query caches changed.
Checklist
docsand added dartdoc comments with///.examplesordocs.Breaking Change?
Related Issues