-
Notifications
You must be signed in to change notification settings - Fork 373
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
Primary caching 19 (final): de-staticify cache globals #4856
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved all of those to re_query
because A) that's the only thing they're used for and B) we have a very nasty dependency cycle otherwise.
crates/re_query/src/util.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Migrated over from re_entity_db
, no changes.
crates/re_query_cache/Cargo.toml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thus fixing our dependency cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just grunt work to adapt to the fact that Caches
needs to be passed around now.
assert!( | ||
self.store_id == *store.id(), | ||
"attempted to use a query cache {} with the wrong datastore ({})", | ||
self.store_id, | ||
store.id(), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I argue this should always be a hard crash; this is just so fundamentally wrong.
Not sure how you would ever get into that situation to begin with either, you'd have to use the store from one StoreHub
and the cache from another...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just grunt work to adapt to the fact that we've been able to remove a superfluous locking layer.
crates/re_query_cache/src/lib.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are now methods.
crates/re_query_cache/src/query.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just grunt work to adapt to the fact that Caches
needs to be passed around now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just grunt work to adapt to the fact that Caches
needs to be passed around now.
crates/re_query_cache/tests/range.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just grunt work to adapt to the fact that Caches
needs to be passed around now.
83a1d39
to
dd80167
Compare
Size changes
|
Range queries used to A) return the frame a T-1, B) accumulate state starting at T-1 and then C) yield frames starting at T. A) was a huge issue for many reasons, which #4793 took care of by eliminating both A) and B). But we need B) for range queries to be context-free, i.e. to be guaranteed that `Range(5, 10)` and `Range(4, 10)` will return the exact same data for frame `5`. This is crucial for multi-tenant settings where those 2 example queries would share the same cache. It also is the nicer-nicer version of the range semantics that we wanted anyway, I just didn't realize back then that it would require so little changes, or I would've gone straight for that. --- Part of the primary caching series of PR (index search, joins, deserialization): - #4592 - #4593 - #4659 - #4680 - #4681 - #4698 - #4711 - #4712 - #4721 - #4726 - #4773 - #4784 - #4785 - #4793 - #4800 - #4851 - #4852 - #4853 - #4856
Simply add a timeless path for the range cache, and actually only iterate over the range the user asked for (we were still blindly iterating over everything until now). Also some very minimal clean up related to #4832, but we have a long way to go... - #4832 --- - Fixes #4821 --- Part of the primary caching series of PR (index search, joins, deserialization): - #4592 - #4593 - #4659 - #4680 - #4681 - #4698 - #4711 - #4712 - #4721 - #4726 - #4773 - #4784 - #4785 - #4793 - #4800 - #4851 - #4852 - #4853 - #4856
45f8d62
to
890dada
Compare
Implement range invalidation and do a quality pass over all the size tracking stuff in the cache. **Range caching is now enabled by default!** - Fixes #4809 - Fixes #374 --- Part of the primary caching series of PR (index search, joins, deserialization): - #4592 - #4593 - #4659 - #4680 - #4681 - #4698 - #4711 - #4712 - #4721 - #4726 - #4773 - #4784 - #4785 - #4793 - #4800 - #4851 - #4852 - #4853 - #4856
dd80167
to
5b4e1d4
Compare
All the grunt work left to get rid of polars. - Remove all helpers and APIs built specifically for polars' `DataFrame`. - Refactor tests that rely on dataframe joins to not require join semantics in the first place (`re_data_store` has no knowledge of those anyway). - The one test that does require join semantics has moved over to `re_query`, where join semantics belong. - All `polars-*` dep have been removed. Don't look at the commit log as it makes no sense: i changed strategies a bunch of times on the way. --- - Part of #4789 - DNR: requires #4856 --- Part of the tiny datatype deduplication PR series: - #4880 - #4883
Ignore `PrimaryNotFound` errors on the cached entity_iterator path, the same way the vanilla path does. - Fixes #4858 - DNR: requires #4856 ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using newly built examples: [app.rerun.io](https://app.rerun.io/pr/4892/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/4892/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [app.rerun.io](https://app.rerun.io/pr/4892/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG - [PR Build Summary](https://build.rerun.io/pr/4892) - [Docs preview](https://rerun.io/preview/bebd894b2f4fc299c9882b9529d24c760b68e0bd/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/bebd894b2f4fc299c9882b9529d24c760b68e0bd/examples) <!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
Before: ![image](https://github.com/rerun-io/rerun/assets/2910679/bec3e40a-529c-443d-9803-9581bfc977da) After: ![image](https://github.com/rerun-io/rerun/assets/2910679/b1507279-7ca3-4c43-b47d-12b88864fdbc) - Fix #4857 - DNR: requires #4856 ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using newly built examples: [app.rerun.io](https://app.rerun.io/pr/4890/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/4890/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [app.rerun.io](https://app.rerun.io/pr/4890/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG - [PR Build Summary](https://build.rerun.io/pr/4890) - [Docs preview](https://rerun.io/preview/06db01a65d711ec7ba802169845e3b79337a91af/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/06db01a65d711ec7ba802169845e3b79337a91af/examples) <!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
The cache will now keep track of missing optional components, and store an empty slice instead of a bunch of `None` values. When queried, an empty shows up as a `None` option to the end-user, who can act appropriately. SFM before: ![image](https://github.com/rerun-io/rerun/assets/2910679/34256f8b-3b4b-4d1a-b1ea-5f9e1fd7860b) SFM after: ![image](https://github.com/rerun-io/rerun/assets/2910679/b676052a-b1fc-4840-bddf-67e5f490add2) --- - Fixes #4779 - DNR: requires #4856 ### What ### Checklist * [x] I have read and agree to [Contributor Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and the [Code of Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md) * [x] I've included a screenshot or gif (if applicable) * [x] I have tested the web demo (if applicable): * Using newly built examples: [app.rerun.io](https://app.rerun.io/pr/4891/index.html) * Using examples from latest `main` build: [app.rerun.io](https://app.rerun.io/pr/4891/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [app.rerun.io](https://app.rerun.io/pr/4891/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json) * [x] The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG - [PR Build Summary](https://build.rerun.io/pr/4891) - [Docs preview](https://rerun.io/preview/bf89c307dac5dc8fd8016dd985f8af30a6ee73a7/docs) <!--DOCS-PREVIEW--> - [Examples preview](https://rerun.io/preview/bf89c307dac5dc8fd8016dd985f8af30a6ee73a7/examples) <!--EXAMPLES-PREVIEW--> - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
⚠️ [Try it live!](https://app.rerun.io/pr/4865/index.html?url=https://storage.googleapis.com/rerun-builds/pull_request/4865/plot_gauss2.rrd) :warning: Make it so users can configure an aggregation strategy in the rare case where they either have so much data or are so zoomed out that most of their plot results in an overdraw blurb. Because this builds on top of the range cache, the data is neatly laid out in a memory slice already so this is very cheap to compute. In my tests, the `MinMax` strategy has worked so well that I've decided to make it the default in the end... That might be controversial :no_mouth:. `Off` vs. `MinMax`, using the [new gaussian walk benchmark](#4903): ![image (26)](https://github.com/rerun-io/rerun/assets/2910679/1811becb-d213-44bb-87ea-0e4a7fa058ad) ![image (27)](https://github.com/rerun-io/rerun/assets/2910679/b8d66c92-8719-4de5-a3cb-72c2ea4b1e96) - Fixes #4271 - DNR: requires #4856
EntityDb
.CacheKey
no longer requires aStoreId
.Part of the primary caching series of PR (index search, joins, deserialization):
VecDeque
extensions #4592FlatVecDeque
#4593LatestAt(T-1)
results into low-level range queries #4793Checklist
main
build: app.rerun.ionightly
build: app.rerun.io