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.

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

The raw uncached query APIs should not exist #6018

Closed
teh-cmc opened this issue Apr 17, 2024 · 2 comments · Fixed by #6036
Closed

The raw uncached query APIs should not exist #6018

teh-cmc opened this issue Apr 17, 2024 · 2 comments · Fixed by #6036
Assignees
Labels
🔍 re_query affects re_query itself

Comments

@teh-cmc
Copy link
Member

teh-cmc commented Apr 17, 2024

We currently have three layers of query APIs:

  • re_data_store, which hosts the very low-level APIs used to fetch raw cells from the store (either latest-at or range).
    These APIs should never be used directly by downstream crates:
    - Nothing should ever query re_data_store directly #6017
  • re_query, which wraps the low-level APIs in a higher level abstraction that takes care of nullability, promise resolution, error handling, etc.
  • re_query_cache, which does everything that re_query does, but also handles caching on top of that.
    re_query_cache is NOT built on top of re_query. It's built atop re_data_store.

The fact that both re_query and re_query_cache exist is a giant PITA.

While they look functionally similar on the surface, they have very different semantics in all aspects that matter upon closer look.
Here's a non exhaustive list of examples:

  • re_query doesn't store any typed data, while re_query_cache does.
  • re_query can easily expose owned data, but not reference data. re_query_cache is the exact opposite.
  • re_query supports fine grained errors within data ranges, re_query_cache on the other hand only supports coarse errors at both ends of the range.
  • re_query_cache exposes data that is protected behind mutex guards, whereas re_query doesn't.
  • The list goes on.

These differences make it impossible to abstract over both re_query and re_query_cache in a way that doesn't completely nullify the advantages of each respective API.
Because we can't abstract these differences away, we have to duplicate a lot of the logic between the two, and this logic ends up behaving in subtly different ways, every time. And because Rust is Rust, those different types and traits poison all neighbor functions and suddenly you have two maintain two separate code bases. It's basically a homegrown case of function coloring.

This creates a never ending stream of issues in the code, which begs the question: why do we have uncached APIs to start with?

The reason is that, because of limitations in our data and caching models, we are currently forced to not cache certain things, such as images and meshes, else the memory overhead would become unmanageable.
The only way to fix these limitations is to address these two issues:

Fixing these issues is no small task.
In the mean time, I might try and see if it's possible to tweak the cached APIs so that we can disable caching for specific components. It's ugly and weird and non-sensical, but it's still infinitely better than the hell we have to deal with right now.

@teh-cmc teh-cmc added the 🔍 re_query affects re_query itself label Apr 17, 2024
@teh-cmc
Copy link
Member Author

teh-cmc commented Apr 17, 2024

In the mean time, I might try and see if it's possible to tweak the cached APIs so that we can disable caching for specific components. It's ugly and weird and non-sensical, but it's still infinitely better than the hell we have to deal with right now.

The one downside of that is that it means keeping track of all raw cells in the CachedRangeResults, as opposed to just the cells that have yet to be deserialized, which is why I stayed away from that path until now.

But now that I have better visibility into the indexing overhead, i realize this is actually completely and irrelevantly small.
And of course, the only sustainable solution is to fix #5967 once and for all anyway.

@teh-cmc teh-cmc self-assigned this Apr 17, 2024
@teh-cmc
Copy link
Member Author

teh-cmc commented Apr 18, 2024

I had a ✨ revelation ✨ last night: what if instead of viewing meshes/images/etc as uncacheable, we viewed them as always to-be-evicted-asap?

That is: hardcode a very simple rule in the cached APIs so that all meshes/images/etc-related entries get unconditionally garbage collected at the start of every frame (something that should be absolutely trivial to do now that our caches are at the component level).
Suddenly the only entry we're caching is the one being shown on the screen right now, which is perfectly fine overhead-wise, since we have to deserialize the data in one place or another anyhow.

That means we can remove the uncached APIs entirely (both re_query & re_query2), and with them, half of our problems.
For downstream consumers, it all looks cached: it's all the same APIs, it's all slices. All the duplicated logic goes away.

You could also get a bit fancier and apply some last-frame-used semantics instead of always evicting unconditionally, but whatever, that kind of fanciness falls into #5974 territory. We can be fancy later.

teh-cmc added a commit that referenced this issue Apr 26, 2024
There is now only one way to query data: `re_query` (well you can still
query the datastore directly if you're a monster, but that's for another
PR).

All queries go through both the query cache and the deserialization
cache.
There will be a follow-up PR to disable the deserialization cache for
specific components.

Most of this is just (re)moving stuff around except for the last two
commits which take care of porting the cached test suites since they
cannot depend on uncached APIs to do comparisons anymore.

- Closes #6018 
- Closes #3320

---

Part of a PR series to completely revamp the data APIs in preparation
for the removal of instance keys and the introduction of promises:
- #5573
- #5574
- #5581
- #5605
- #5606
- #5633
- #5673
- #5679
- #5687
- #5755
- #5990
- #5992
- #5993 
- #5994
- #6035
- #6036
- #6037

Builds on top of the static data PR series:
- #5534
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔍 re_query affects re_query itself
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant