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.

Sign up for GitHub

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

Jump to bottom

Primary caching 13: stats & memory panel integration for range queries #4785

Merged
merged 5 commits into from
Jan 15, 2024
Merged

Primary caching 13: stats & memory panel integration for range queries #4785

merged 5 commits into from
Jan 15, 2024

Conversation

teh-cmc
Copy link
Member

@teh-cmc teh-cmc commented Jan 11, 2024 •
edited by github-actions bot
Loading

Title.

24-01-11_13.21.53.patched.mp4

Part of the primary caching series of PR (index search, joins, deserialization):

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

@teh-cmc teh-cmc added 🔍 re_query affects re_query itself 📉 performance Optimization, memory use, etc do-not-merge Do not merge this PR exclude from changelog PRs with this won't show up in CHANGELOG.md labels Jan 11, 2024
This was referenced Jan 11, 2024
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@teh-cmc teh-cmc force-pushed the cmc/primcache_13_range_stats branch 2 times, most recently from a9af62b to 77a0113 Compare January 12, 2024 07:37
This was referenced Jan 12, 2024
Merged
Merged
@Wumpf Wumpf self-requested a review January 12, 2024 16:18
Wumpf
Wumpf previously requested changes Jan 12, 2024
View reviewed changes
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking great, but that global switch weirds me out 😄

crates/re_query_cache/src/cache_stats.rs Outdated
@@ -20,6 +20,19 @@ pub fn set_detailed_stats(b: bool) {
ENABLE_DETAILED_STATS.store(b, std::sync::atomic::Ordering::Relaxed);
}

/// If `true`, will show stats about empty caches too, which likely indicates a bug (dangling bucket).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meaning empty caches should be removed automatically?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find it a bit weird to encounter this atomic here. Isn't that just a property of the ui? We should put it on the MemoryPanel struct instead.
Personally, I'd just always enable this anyways doesn't seem to hurt 🤷

Copy link
Member Author

@teh-cmc teh-cmc Jan 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

meaning empty caches should be removed automatically?

Yes, or not exist to begin with: either way, empty buckets are bug and we want to be able to see it.

I find it a bit weird to encounter this atomic here. Isn't that just a property of the ui? We should put it on the MemoryPanel struct instead.

I just blindly followed how the other memory panel checkboxes are implemented 🤷‍♂️.
I'll see if I can easily make it a part of the panel.

Personally, I'd just always enable this anyways doesn't seem to hurt 🤷

No it's insanely costly with a real dataset

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for upgrading all of them, liking this much better :)

teh-cmc added a commit that referenced this pull request Jan 15, 2024
@teh-cmc
Primary caching 7: Always expose the data time in query responses (#4711
8dd1681 
)

_99% grunt work, the only somewhat interesting thing happens in
`query_archetype`_

Our query model always operates with two distinct timestamps: the
timestamp you're querying for (`query_time`) vs. the timestamp of the
data you get back (`data_time`).

This is the result of our latest-at semantics: a query for a point at
time `10` can return a point at time `2`.
This is important to know when caching the data: a query at time `4` and
a query at time `8` that both return the data at time `2` must share the
same single entry or the memory budget would explode.

This PR just updates all existing latest-at APIs so they return the data
time in their response.
This was already the case for range APIs.

Note that in the case of `query_archetype`, which is a compound API that
emits multiple queries, the data time of the final result is the most
recent data time among all of its components.

A follow-up PR will use the data time to deduplicate entries in the
latest-at cache.

---

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
teh-cmc added a commit that referenced this pull request Jan 15, 2024
@teh-cmc
Primary caching 8: implement latest-at data-time cache entry deduplic…
ccfd21a 
…ation (#4712)

Introduces the notion of cache deduplication: given a query at time `4`
and a query at time `8` that both returns data at time `2`, they must
share a single cache entry.

I.e. starting with this PR, scrubbing through the OPF example will not
result if more cache memory being used.

---

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
teh-cmc added a commit that referenced this pull request Jan 15, 2024
@teh-cmc
Primary caching 9: timeless latest-at support (#4721)
f569223 
Introduces a dedicated cache bucket for timeless data and properly
forwards the information through all APIs downstream.

---

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
teh-cmc added a commit that referenced this pull request Jan 15, 2024
@teh-cmc
Primary caching 10: latest-at cache invalidation (#4726)
68de772 
This implements cache invalidation via a `StoreSubscriber`.

We keep track of the timestamps to invalidate in the `StoreSubscriber`,
but we only do the actual removal of components at query time.
This is similar to how we handle bucket sorting in the main store: doing
it at query time has the benefit that the frame time effectively behaves
as natural micro-batching mechanism that vastly improves performance.

---

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
teh-cmc added a commit that referenced this pull request Jan 15, 2024
@teh-cmc
Primary caching 11: cache stats and integration with memory panel (#4773
1f19805 
)

The primary cache now tracks memory statistics and display them in the
memory panel.

This immediately highlights a very stupid thing that the cache does:
missing optional components that have been turned into streams of
default values by the `ArchetypeView` are materialized as such
:man_facepalming:
- #4779


https://github.com/rerun-io/rerun/assets/2910679/876b264a-3f77-4d91-934e-aa8897bb32fe



- Fixes #4730 


---

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
@teh-cmc teh-cmc force-pushed the cmc/primcache_12_barebone_range branch from 17b548b to 5fb6abd Compare January 15, 2024 12:26
Base automatically changed from cmc/primcache_12_barebone_range to main January 15, 2024 14:49
teh-cmc added a commit that referenced this pull request Jan 15, 2024
@teh-cmc
Primary caching 12: barebone range support (#4784)
886014a 
**Prefer on a per-commit basis, stuff has moved around**

Range queries are back!... in the most primitive form possible.

No invalidation, no bucketing, no optimization, no nothing. Just putting
everything in place.


https://github.com/rerun-io/rerun/assets/2910679/a65281e4-9843-4598-9547-ce7e45197995

---

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
@teh-cmc teh-cmc force-pushed the cmc/primcache_13_range_stats branch from 77a0113 to a79d00d Compare January 15, 2024 14:51
@teh-cmc teh-cmc removed the do-not-merge Do not merge this PR label Jan 15, 2024
@teh-cmc teh-cmc force-pushed the cmc/primcache_13_range_stats branch from 9ef1839 to 53e051a Compare January 15, 2024 15:15
@teh-cmc teh-cmc merged commit bdae240 into main Jan 15, 2024
22 of 31 checks passed
@teh-cmc teh-cmc deleted the cmc/primcache_13_range_stats branch January 15, 2024 15:17
teh-cmc added a commit that referenced this pull request Jan 15, 2024
@teh-cmc
Primary caching 14: don't bake LatestAt(T-1) results into low-level…
dc8cf2d 
… range queries (#4793)

Our low-level range APIs used to bake the latest-at results at
`range.min - 1` into the range results, which is a big problem in a
multi tenant setting because `range(1, 10)` vs. `latestat(1) + range(2,
10)` are two completely different things.

Side-effect: a plot with a window of len 1 now behaves as expected:



https://github.com/rerun-io/rerun/assets/2910679/957ac367-35a6-4bea-9f40-59d51c556639

---

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
teh-cmc added a commit that referenced this pull request Jan 15, 2024
@teh-cmc
Primary caching 15: range read performance optimization (#4800)
04f8534 
The most obvious and most important performance optimization when doing
cached range queries: only upsert data at the edges of the bucket /
ring-buffer.

This works because our buckets (well, singular, at the moment) are
always dense.

- #4793  

![image](https://github.com/rerun-io/rerun/assets/2910679/7246827c-4977-4b3f-9ef9-f8e96b8a9bea)
- #4800:

![image](https://github.com/rerun-io/rerun/assets/2910679/ab78643b-a98b-4568-b510-2b8827467095)

---

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
This was referenced Jan 18, 2024
Merged
Merged
Merged
Merged
teh-cmc added a commit that referenced this pull request Jan 23, 2024
@teh-cmc
Primary caching 16: context-free range semantics (#4851)
20c768f 
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
teh-cmc added a commit that referenced this pull request Jan 23, 2024
@teh-cmc
Primary caching 17: timeless range (#4852)
5800d1a 
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
teh-cmc added a commit that referenced this pull request Jan 23, 2024
@teh-cmc
Primary caching 18: range invalidation (ENABLED BY DEFAULT 🎊) (#4853)
36446b6 
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
teh-cmc added a commit that referenced this pull request Jan 23, 2024
@teh-cmc
Primary caching 19 (final): de-statification (#4856)
4444217 
- Quick sanity pass over all the intermediary locks and refcounts to
make sure we don't hold anything for longer than we need.
- Get rid of all static globals and let the caches live with their
associated stores in `EntityDb`.
- `CacheKey` no longer requires a `StoreId`.

---

- Fixes #4815 

---

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
@teh-cmc teh-cmc added include in changelog and removed exclude from changelog PRs with this won't show up in CHANGELOG.md labels Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wumpf Wumpf Wumpf left review comments

Assignees
No one assigned
Labels
include in changelog 📉 performance Optimization, memory use, etc 🔍 re_query affects re_query itself
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@teh-cmc @Wumpf