Skip to content

Use new internals.weak module to unify caching mechanisms in evaluate - #745

Merged
jfeser merged 4 commits into
masterfrom
eb-unify-cache
Aug 10, 2026
Merged

Use new internals.weak module to unify caching mechanisms in evaluate#745
jfeser merged 4 commits into
masterfrom
eb-unify-cache

Conversation

@eb8680

@eb8680 eb8680 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

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.

@eb8680
eb8680 requested a review from jfeser August 4, 2026 16:26
@eb8680 eb8680 added the blocked label Aug 4, 2026

@jfeser jfeser left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you summarize the changes to the caching semantics? On master, I understand that we have two forms of caching:

  1. Side-effect free interpretations have an opt-in cache stored on the term with an indefinite lifetime.
  2. 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?

@eb8680

eb8680 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

After this PR, there is a context manager effectful.internals.runtime.cache that determines cache lifetime:

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 x1

Caching within a single evaluate call is still always on, preserving the behavior introduced in #594. It uses the active cache if there is one and creates an ephemeral one if there isn't.

@jfeser

jfeser commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

I think we should keep the term-level caching for pure interpretations. It's completely transparent to users (ditto the internal caching in evaluate). Most of our observed performance improvement comes from caching typeof and friends.

Also, if we expect users to place cache contexts for performance, we should move it out of internals.

@eb8680

eb8680 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

This PR removes PureInterpretation and Term-owned caches, it's not straightforward to restore that behavior without the simplifications to fvsof and sizesof in #747 .

Also, that behavior didn't extend to data structures, which is the primary use case in RoboTL, and many builtin collections like tuples, lists and dicts aren't weakref.ref-able. That means caching e.g. fvsof(list(...)) requires keeping a strong reference to the list(...) in the cache, which prevents its elements from being garbage-collected. I think it's more correct in that case to require an explicit cache context, so users know and control exactly when those strong references are dropped and their memory freed.

@jfeser

jfeser commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

The point about data structures is a good one. The previous implementation partially recomputes analyses in the presence of collections.

@jfeser
jfeser self-requested a review August 6, 2026 15:12
@eb8680 eb8680 linked an issue Aug 6, 2026 that may be closed by this pull request
Base automatically changed from eb-weak-module to master August 10, 2026 16:39
@jfeser
jfeser merged commit 61e4da3 into master Aug 10, 2026
29 checks passed
@jfeser
jfeser deleted the eb-unify-cache branch August 10, 2026 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Clarify evaluation semantics of shared terms

2 participants