Use new internals.weak module to unify caching mechanisms in evaluate - #745
Conversation
jfeser
left a comment
There was a problem hiding this comment.
Could you summarize the changes to the caching semantics? On master, I understand that we have two forms of caching:
- Side-effect free interpretations have an opt-in cache stored on the term with an indefinite lifetime.
- There is a cache scoped to the duration of a single call to evaluate (including nested calls to evaluate) that is keyed on term identity.
What do we have after this PR?
|
After this PR, there is a context manager with cache() as eval_cache:
x1 = foo()
assert foo() is not x1
assert evaluate(x1) is x1
# only innermost cache() is active
with cache() as eval_cache2:
x2 = evaluate(x1)
assert x2 is not x1
assert evaluate(x1) is not x1
# no caching outside cache()
assert evaluate(x1) is not evaluate(x1)
with cache(cache=eval_cache2):
assert evaluate(x1) is not x1
assert evaluate(x2) is x2
with cache(cache=eval_cache) as eval_cache:
assert evaluate(x1) is x1Caching within a single |
|
I think we should keep the term-level caching for pure interpretations. It's completely transparent to users (ditto the internal caching in Also, if we expect users to place |
|
This PR removes Also, that behavior didn't extend to data structures, which is the primary use case in RoboTL, and many builtin collections like |
|
The point about data structures is a good one. The previous implementation partially recomputes analyses in the presence of collections. |
Factored out of #743 for ease of review. Blocked by #744, which it is stacked on top of.
This PR uses the new functionality added in #744 to unify and generalize the caching mechanisms introduced in #594 #716 #726. Unlike #744 it does include some breaking changes to the behavior as of #716 #726.