Skip to content
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

Go API: Allow providing custom base cache #7329

Merged
merged 1 commit into from
Jan 30, 2025

Conversation

anderseknert
Copy link
Member

The same way it's possible to provide a VirtualCache, it should be possible to bring your own BaseCache. In clients like Regal (when invoked via regal lint), the base data is only ever loaded once and doesn't change later. Yet currently, each evaluation (of which there are hundreds) will have a new cache instantiated, leading to unnecessary cache misses.

Since our inmem-store is AST-based, we could also try to have the base cache tap directly into the store for a 100% hit ratio, avoiding the more costly storage queries. But that's still untested this point 🤓

(The tiny changes in resolver.go are unrelated to this feature but fix a perf issue I noticed last night as I was testing that functionality. Too small fix to warrant a PR of its own.)

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

This reminds me, have you ever tested the new AST-Values-in-inmem-storage feature that shipped some time ago?

The same way it's possible to provide a VirtualCache, it should be
possible to bring your own BaseCache. In clients like Regal (when
invoked via `regal lint`), the base data is only ever loaded once and
doesn't change later. Yet currently, each evaluation (of which there are
hundreds) will have a new cache instantiated, leading to unnecessary
cache misses.

Since our inmem-store is AST-based, we could also try to have the base
cache tap directly into the store for a 100% hit ratio, avoiding the
more costly storage queries. But that's still untested this point 🤓

(The tiny changes in resolver.go are unrelated to this feature but fix
a perf issue I noticed last night as I was testing that functionality.
Too small fix to warrant a PR of its own.)

Signed-off-by: Anders Eknert <[email protected]>
@anderseknert
Copy link
Member Author

anderseknert commented Jan 30, 2025

This reminds me, have you ever tested the new AST-Values-in-inmem-storage feature that shipped some time ago?

Yes! It's enabled in Regal and helps quite a bit! That's what I was alluding to here:

Since our inmem-store is AST-based, we could also try to have the base cache tap directly into the store for a 100% hit ratio, avoiding the more costly storage queries. But that's still untested this point 🤓

Meaning even with that enabled, going to the cache is much cheaper than going via the storage API, where a single Read request costs 1 + x allocations just to convert a ref to path and back (where x is the length of the ref). That used to be 1 + 2x but was made a little cheaper recently by pooling. And that's just for the path — there are also costs attached to boxing values to interface{}, etc. The cache OTOH is cheap. Only downside is of course that it's somewhat silly to store the same data in the same format in two different places and call one of them a cache. That's why I'm thinking we could have our custom base cache utilize the same backing AST object as the store does. It would mean we have only one copy of the data, and that we avoid the cost of the storage API.

(EDIT: For the regal lint case, I think we could even skip providing a store entirely and load all the data into the base cache directly. That won't work for the language server though, as in that case the data is updated and we need to make sure that's transactional. We can make it so I guess, but then we've reinvented the storage API, lol)

The better solution would be a V2 storage API, and I think @johanfylling wished for that too after he worked on the AST backed inmem store. But that's for another day :)

@anderseknert anderseknert merged commit b751182 into open-policy-agent:main Jan 30, 2025
28 checks passed
@anderseknert anderseknert deleted the basecache branch January 30, 2025 12:03
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.

3 participants