-
Notifications
You must be signed in to change notification settings - Fork 186
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
Add a function Evaluator class with policy based caching capabilities #5253
base: dev
Are you sure you want to change the base?
Conversation
95dab0c
to
b986936
Compare
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.
Needs more documentation in certain places. I didn't notice any defects overall. The tests, however, and not table-driven and thus not as extensive as this code deserves.
/** | ||
* Abstract class for caching policies. | ||
* This contains most of the logic of the caching mechanism | ||
* except budgeting which is policy specific. | ||
* | ||
* @tparam Key The type of key | ||
* @tparam Value The type of value | ||
*/ |
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.
The class invariants of the three containers should be documented here in the class header. Because they interact with each other, documenting them one at a time is insufficient to fully describe the class behavior.
throw std::logic_error( | ||
"The memory consumed by this value exceeds the budget of the " | ||
"cache."); |
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.
Throwing if the cache is empty and the object still won't fit feels like incorrect behavior. In a case where there's no room in the cache, an evaluator should still evaluate, even if it can't cache the result.
If it's not going to act like that, documentation to that effect belongs in the class header, since it's otherwise a major surprise.
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.
This is a very nice catch and I agree the behavior should be to evaluate but not cache. Logging a warning here seems appropriate?
9733e59
to
2c93955
Compare
a823c9c
to
8d48d60
Compare
#5215 for context and previous reviews.
This work evolved into something a bit more generic than the tile offsets transparent cache we originally intended to write.
This PR adds:
Evaluator
classImmediateEvaluation
policyConfigures the evaluator to execute
cb(key)
for any invocation ofeval(key)
.MaxEntriesCache
policyConfigures the evaluator to execute
cb(key)
and cache the results in a LRU cachethat cannot hold more than
N
values. Caching valueN+1
triggers cache eviction.Evaluator<MaxEntriesCache<Key, Value, N>, Callback> eval(cb);
MemoryBudgetedCache
policyConfigures the evaluator to execute
cb(key)
and cache the results in a LRU cachewith a capacity of
M
bytes whereM
is passed during the evaluator construction.[sc-51496]
TYPE: FEATURE
DESC: Add a function Evaluator class with policy based caching capabilities