Generalize the GPU brush cache for reusable node data - #4526
YohYamasaki wants to merge 3 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
653ac2b to
50447d6
Compare
50447d6 to
3f6f2a9
Compare
0HyperCube
left a comment
0HyperCube
left a comment
There was a problem hiding this comment.
Seems reasonable. I'm not very good at reasoning about generic ZSTs 🙃.
| nonce: u64, // Avoid deduplication of cache entries across different brush nodes. | ||
| } | ||
|
|
||
| impl<K: Copy + PartialEq, M: CachePolicy<K>> Cache<K, M> { |
There was a problem hiding this comment.
Why is the CachePolicy generic M? I'd suggest a name longer than one character might make it easier to understand what is going on?
| /// Removes and returns the value stored for `key`. | ||
| /// Returns `None` if the key is absent or the stored value has a different type. | ||
| /// A type mismatch leaves the original value cached. | ||
| pub fn take<S: std::any::Any + Send + Sync>(&self, key: &K) -> Option<S> { |
There was a problem hiding this comment.
Why is the value stored S? Surely V would be better to match HashMap<K, V>?
| if slot.epoch == self.epoch { | ||
| self.epoch += 1; | ||
| } |
There was a problem hiding this comment.
@timon-schelling this is a bit confusing. Why only increment the epoch if we have a cache hit? If you have infinite cache misses then you end up still on epoch 0?
| }) | ||
| }); | ||
|
|
||
| M::retire(&mut self.entries, &mut self.policy_state); |
There was a problem hiding this comment.
Why retire when taking since the new size of the cache is ≤ the old size?
2 participants
This extracts
BrushCachefrom #4468 into a reusable generic cache, which will also be used in the mesh gradient #4081. It now allows to have custom eviction strategy through cache policies.Also fixed a bug-ish type mismatch path in
Cache::take()that could trigger duplicate eviction policy updates by removing and reinserting the entry.