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

Copy validated declarations between checker.Env instances on extension #347

Merged
merged 10 commits into from Mar 24, 2022

Conversation

TristonianJones
Copy link
Collaborator

@TristonianJones TristonianJones commented Apr 30, 2020

This change introduces several small changes to improve environment extension and CEL bootstrapping.

Type-substitution Utility

Introduce a common helper method for performing type substitution checks. The helper method scopes
the use of proto.Equal within internalIsAssignable to only the cases where recursive type substitutions
occur. All other branches progressively narrow the types under consideration and use simpler checks
appropriate for the protobuf type.

This change improves check-time performance roughly 40% per issue #346.

EagerlyValidateDeclarations Option

When this option is enabled, a guaranteed to compile expression is parsed and type-checked ensuring that
the internal instance of the checker.Env is initialized and all declarations provided as options to the
cel.NewEnv() call are checked to ensure there are no overlapping / colliding function declarations.

Using this option in combination with Extend also makes it possible to copy the already validated
declarations into the cloned checker.Env of the extended environment. The cloned declarations are
shallowly copied on checker.Env initialization. If a declaration provided to the derived environment
needs to be merged with a function type declared in an ancestor environment, the ancestor function
is copied prior to being modified to ensure changes are isolated to the derived environment.

NewEnv() Extends by Default

The NewEnv() call now extends a globally initialized copy of the CEL standard environment whose
declarations have been validated as part of an init() call. This means in the common case that when
only a few declarations or parameters are changed as part of the CEL standard environment, the internal
checker.Env setup cost is kept to a minimum.

Filtered Overloads

The support for CrossNumericTypes was filtering overloads on addition to the checker.Env. This
behavior has now been shifted to filtering on access of the overload set within the type-checker.
Without this change it was impossible to toggle CrossNumericTypes support between Extend
calls.

Performance

The performance impact of these changes is dramatic, dropping the cost of the simple case by >95%

benchmark                          old ns/op     new ns/op     delta
BenchmarkNewEnvLazy-8              14748         13535         -8.22%
BenchmarkNewEnvEager-8             2506297       61987         -97.53%
BenchmarkEnvExtendEager-8          2658853       62218         -97.66%
BenchmarkEnvExtendEagerTypes-8     2742668       65600         -97.61%
BenchmarkEnvExtendEagerDecls-8     2650621       102302        -96.14%

benchmark                          old allocs     new allocs     delta
BenchmarkNewEnvLazy-8              97             39             -59.79%
BenchmarkNewEnvEager-8             20283          209            -98.97%
BenchmarkEnvExtendEager-8          20300          204            -99.00%
BenchmarkEnvExtendEagerTypes-8     20205          221            -98.91%
BenchmarkEnvExtendEagerDecls-8     20409          474            -97.68%

benchmark                          old bytes     new bytes     delta
BenchmarkNewEnvLazy-8              10489         7634          -27.22%
BenchmarkNewEnvEager-8             578535        18031         -96.88%
BenchmarkEnvExtendEager-8          581087        17801         -96.94%
BenchmarkEnvExtendEagerTypes-8     576225        20088         -96.51%
BenchmarkEnvExtendEagerDecls-8     579028        27177         -95.31%

@TristonianJones TristonianJones changed the title Refactor the type substitution checks for simplificity and performance Refactor the type substitution checks for simplicity and performance Apr 30, 2020
@TristonianJones
Copy link
Collaborator Author

@JimLarson Do you think you'd be able to take a look at this? I realize more extensive refactors are probably necessary, but looking this change again, it does make the code easier to decipher (imo).

Copy link
Contributor

@JimLarson JimLarson left a comment

Choose a reason for hiding this comment

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

I don't think I'll be able to reload the state in my head to make sense of this anytime soon, so approving to get it off my review list and unblock you.

checker/types.go Outdated Show resolved Hide resolved
…ed declarations from one checker.Env to derivations created using Extend
@TristonianJones TristonianJones marked this pull request as ready for review March 23, 2022 23:20
checker/env.go Show resolved Hide resolved
checker/env.go Show resolved Hide resolved
checker/env.go Show resolved Hide resolved
Copy link
Collaborator

@jpbetz jpbetz left a comment

Choose a reason for hiding this comment

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

LGTM

I added a few comments, mostly questions and one optional change.

The test coverage and benchmarks look solid. This covers the use case needed by Kubernetes really well.

I did not review the checker/types.go. I don't feel like I understand the specifics well enough there to be helpful. All the validatedDeclarations code made sense to me.

@TristonianJones TristonianJones changed the title Refactor the type substitution checks for simplicity and performance Copy validated declarations between checker.Env instances on extension Mar 24, 2022
@@ -432,6 +462,16 @@ func (e *Env) configure(opts []EnvOption) (*Env, error) {
if err != nil {
return nil, err
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems...hacky. But okay.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes, the compile is a bit hacky. Let me just do the checker construction here.

checker/env.go Outdated Show resolved Hide resolved
checker/types.go Show resolved Hide resolved
@TristonianJones
Copy link
Collaborator Author

@jpbetz Once the release is cut would you mind just updating the #519 ticket with the updated performance graph?

Closes #346

@TristonianJones TristonianJones merged commit 1287953 into google:master Mar 24, 2022
@TristonianJones TristonianJones deleted the checker-perf-ixxue branch March 24, 2022 02:55
@jpbetz
Copy link
Collaborator

jpbetz commented Mar 24, 2022

@jpbetz Once the release is cut would you mind just updating the #519 ticket with the updated performance graph?

I'd be happy to.

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.

None yet

4 participants