Skip to content

perf: cache InvocationKey hash values at construction time#105

Closed
arirubinstein wants to merge 1 commit intoopen-policy-agent:mainfrom
arirubinstein:eng/ari/cache-invocationkey
Closed

perf: cache InvocationKey hash values at construction time#105
arirubinstein wants to merge 1 commit intoopen-policy-agent:mainfrom
arirubinstein:eng/ari/cache-invocationkey

Conversation

@arirubinstein
Copy link
Copy Markdown
Contributor

What code changed, and why?

Pre-compute and cache the hash value for InvocationKey when it is
constructed, rather than re-hashing funcName and args on every
dictionary lookup.

Why: InvocationKey is used as the key for the memoization cache
(MemoCache). Each memo lookup calls hash(into:), which traverses
the funcName string and the entire args array of Operands. Since
InvocationKey is immutable (all let fields) and typically looked up
multiple times per construction, hashing the same data repeatedly is
pure overhead.

By computing the hash once in init() and returning it from
hash(into:), dictionary lookups become O(1) integer operations
instead of O(n) string+array traversals.

koponen
koponen previously approved these changes Feb 26, 2026
Copy link
Copy Markdown
Contributor

@koponen koponen left a comment

Choose a reason for hiding this comment

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

LGTM!

@arirubinstein arirubinstein force-pushed the eng/ari/cache-invocationkey branch 2 times, most recently from 5a27693 to 666fc8a Compare February 26, 2026 23:49
Comment thread Sources/Rego/IREvaluator.swift
@arirubinstein arirubinstein force-pushed the eng/ari/cache-invocationkey branch 2 times, most recently from e79d825 to 59d1916 Compare February 27, 2026 06:34
@koponen koponen force-pushed the eng/ari/cache-invocationkey branch from 59d1916 to ac8d5aa Compare February 27, 2026 17:33
koponen
koponen previously approved these changes Feb 27, 2026
@koponen
Copy link
Copy Markdown
Contributor

koponen commented Feb 27, 2026

Interestingly, this together with the other hash precompute patch does result in small regression our benchmarks, in particular array iterations slow down quite a bit, whereas collection building improves a bit. Would you have a benchmark to add where this patch shines?

@arirubinstein
Copy link
Copy Markdown
Contributor Author

I believe the benefit of this PR in isolation is still valuable, however I'm going to re-write the implementation for #109 to use ordinal hashing instead, which should cut the allocation cost a bit. In addition, I will add a new benchmark to cover Memoization - Overlapping Rules, which should light that path up a bit more in the benchmark

Comment thread Sources/Rego/IREvaluator.swift Outdated
Signed-off-by: Ari Rubinstein <arirubinstein@users.noreply.github.com>
@arirubinstein arirubinstein force-pushed the eng/ari/cache-invocationkey branch from 5ec7e3c to b64bb5b Compare February 28, 2026 18:10
@philipaconrad philipaconrad self-assigned this Mar 17, 2026
@philipaconrad
Copy link
Copy Markdown
Member

@arirubinstein I've assigned myself on this PR so that it won't go away on my notifications until it closes or merges. I'll see about getting a review pass in this afternoon.

Copy link
Copy Markdown
Member

@philipaconrad philipaconrad left a comment

Choose a reason for hiding this comment

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

Thanks @arirubinstein for putting this PR together! 😄

I greatly appreciate the test cases (makes it easier to know things work as intended), and I agree with dropping the custom Equatable implementation-- it's subsumed by the Hashable implementation that's already there.

@philipaconrad
Copy link
Copy Markdown
Member

@koponen Could you take a look at this PR, and see if we still need it?

@koponen
Copy link
Copy Markdown
Contributor

koponen commented Apr 22, 2026

@koponen Could you take a look at this PR, and see if we still need it?

It seems less critical now as the current version of this structure has merely three (u)int32s in it. Let's re-evaluate if the hashing still pops up.

@philipaconrad
Copy link
Copy Markdown
Member

Thanks @koponen, that makes sense. Thanks @arirubinstein for putting together the original PR, but I think we'll close this one for the time being, and resurrect it if hashing becomes an issue again in profiles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants