red-knot: introduce a StatisticsRecorder trait for the KeyValueCache#11179
red-knot: introduce a StatisticsRecorder trait for the KeyValueCache#11179AlexWaygood merged 2 commits intoastral-sh:mainfrom
red-knot: introduce a StatisticsRecorder trait for the KeyValueCache#11179Conversation
crates/red_knot/src/cache.rs
Outdated
| pub struct KeyValueCache<K, V, S = DefaultStatisticsRecorder> | ||
| where | ||
| S: StatisticsRecorder, | ||
| { | ||
| map: FxDashMap<K, V>, | ||
| statistics: CacheStatistics, | ||
| statistics: S, | ||
| } |
There was a problem hiding this comment.
I like the trait, but making KeyValueCache generic over S feels heavyweight because it increases the complexity of using the type (yes, it defaults to a type, but you still get scared away by three generics when looking at the type).
There was a problem hiding this comment.
Makes sense. Is there a way of using a trait here without making KeyValueCache generic? If not, I'm not wedded to this approach -- happy to do something else with this, or not do anything at all!
There was a problem hiding this comment.
I suppose I could do statistics: Box<dyn StatisticsRecorder>?
There was a problem hiding this comment.
You could, but that requires an allocation and adds an overhead to cache lookups (even when it's the release statistics).
I think we should not over-engineer this. We might even go with always having statistics. This is just some prototype code.
There was a problem hiding this comment.
Makes sense. Should I just close this for now, then? :-)
There was a problem hiding this comment.
I like the trait. I would just remove the type param.
There was a problem hiding this comment.
Ohh, I think I see what you mean. Sorry for being dense.
|
|
Going to let @MichaReiser confirm if this looks ok. |
3 participants
Summary
This PR changes the
DebugStatisticsandReleaseStatisticsstructs so that they implement a commonStatisticsRecordertrait, and makes theKeyValueCachestruct generic over a type parameter bound to that trait. The advantage of this approach is that it's much harder for theDebugStatisticsandReleaseStatisticsstructs to accidentally grow out of sync in the methods that they implement, which was the cause of the release-build failure recently fixed in #11177.Test Plan
cargo test -p red_knotandcargo build --releaseboth continue to pass for me locally