-
Notifications
You must be signed in to change notification settings - Fork 1.6k
red-knot: introduce a StatisticsRecorder trait for the KeyValueCache
#11179
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
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose I could do statistics: Box<dyn StatisticsRecorder>?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Should I just close this for now, then? :-)
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 like the trait. I would just remove the type param.
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.
Ohh, I think I see what you mean. Sorry for being dense.
|
|
Going to let @MichaReiser confirm if this looks ok. |
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