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

perf: cache schema types #6970

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

nikpivkin
Copy link
Contributor

This PR adds caching of schema types.

Fixes: #6969

@nikpivkin nikpivkin marked this pull request as ready for review August 28, 2024 09:49
@simar7
Copy link

simar7 commented Aug 28, 2024

@anderseknert any ETA on when we would be able to land this? Currently we'd have to implement some workarounds prior to this getting landed for our next release.

@anderseknert
Copy link
Member

Looks like a small enough change to be able to land before the next release goes out this week. @ashutosh-narkar and @johanfylling's call.

ast/check.go Outdated
continue
}

ref := schemaAnnot.Path
Copy link
Member

Choose a reason for hiding this comment

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

We can move this above

if refType == nil {
	continue
}

and keep the original check

if ref == nil && refType == nil {
    continue
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Member

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

This change looks good. It would be helpful if you could add a test case.

@anderseknert
Copy link
Member

The type checker can't be accessed concurrently, right? 😅

@ashutosh-narkar
Copy link
Member

ashutosh-narkar commented Aug 28, 2024

The type checker can't be accessed concurrently, right? 😅

There are existing map fields and I don't see a mutex so maybe not. Or maybe we have one at the call site. But I haven't checked the code.

Copy link

netlify bot commented Aug 29, 2024

Deploy Preview for openpolicyagent ready!

Name Link
🔨 Latest commit f761af5
🔍 Latest deploy log https://app.netlify.com/sites/openpolicyagent/deploys/66d7767e275be300081a9d95
😎 Deploy Preview https://deploy-preview-6970--openpolicyagent.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nikpivkin
Copy link
Contributor Author

@ashutosh-narkar How can you test caching if it's an internal implementation? Need to add performance tests?

@nikpivkin
Copy link
Contributor Author

The tests on darwin failed, but from the logs I don't see why.

@ashutosh-narkar
Copy link
Member

@ashutosh-narkar How can you test caching if it's an internal implementation? Need to add performance tests?

If we could add some unit tests for checkRule for example that exercise the cache, that would be great.

@simar7
Copy link

simar7 commented Aug 30, 2024

The tests on darwin failed, but from the logs I don't see why.

@ashutosh-narkar @anderseknert do you know why the tests are failing? We don't see results in the CI log output stating anything is failing.

@nikpivkin
Copy link
Contributor Author

Hi @ashutosh-narkar . The tests already cover all cache use cases. How can a test look like, without breaking encapsulation and explicitly checking for the presence of values in the cache?

Signed-off-by: Nikita Pivkin <[email protected]>
@ashutosh-narkar ashutosh-narkar merged commit 76f7038 into open-policy-agent:main Sep 3, 2024
28 checks passed
@nikpivkin nikpivkin deleted the schema-types branch September 16, 2024 05:27
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.

[Perf] Cache schema types
4 participants