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

Generate actions HashMap at ValidatorSchema instantiation #1290

Merged

Conversation

tpaulus
Copy link
Contributor

@tpaulus tpaulus commented Oct 24, 2024

Description of changes

This change fixes part of the issues identified in #1285 by pulling up the expensive action inversion from CoreSchema construction to ValidatorSchema construction, which is only done once per Schema.

Checklist for requesting a review

The change in this PR is (choose one, and delete the other options):

  • A change "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.)

I confirm that this PR (choose one, and delete the other options):

  • Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).

I confirm that cedar-spec (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar formal model or DRT infrastructure.

I confirm that docs.cedarpolicy.com (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar language specification.

Signed-off-by: Tom Paulus <tpaulus@cloudflare.com>
@tpaulus tpaulus force-pushed the 1285-core-schema-performance branch from 5fb58b5 to 80b7ebc Compare October 24, 2024 18:18
@tpaulus tpaulus marked this pull request as ready for review October 24, 2024 18:19
@john-h-kastner-aws john-h-kastner-aws self-requested a review October 24, 2024 18:21
Signed-off-by: Tom Paulus <tpaulus@cloudflare.com>
@tpaulus tpaulus force-pushed the 1285-core-schema-performance branch from 6208ac7 to 2ef2f90 Compare October 25, 2024 16:32
Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws left a comment

Choose a reason for hiding this comment

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

Looks good.

cedar-policy-validator/src/coreschema.rs Outdated Show resolved Hide resolved
Signed-off-by: Tom Paulus <tpaulus@cloudflare.com>
@tpaulus tpaulus force-pushed the 1285-core-schema-performance branch from 2f5e11f to 02db858 Compare October 25, 2024 17:09
cedar-policy/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Adrian Palacios <73246657+adpaco-aws@users.noreply.github.com>
Signed-off-by: Tom Paulus <tpaulus@cloudflare.com>
@tpaulus tpaulus force-pushed the 1285-core-schema-performance branch from fc7fd1a to c2dc40a Compare October 25, 2024 17:42
Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws left a comment

Choose a reason for hiding this comment

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

looks great, thanks! Will merge after CI runs. (Java build failure is expected)

@john-h-kastner-aws john-h-kastner-aws merged commit 44998fe into cedar-policy:main Oct 25, 2024
17 of 19 checks passed
@tpaulus tpaulus deleted the 1285-core-schema-performance branch October 25, 2024 17:54
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