-
Notifications
You must be signed in to change notification settings - Fork 234
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
Copy validated declarations between checker.Env instances on extension #347
Conversation
@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). |
There was a problem hiding this 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.
…e. This simple change roughly doubles check-time performance
5196136
to
fa161e5
Compare
fa161e5
to
633712c
Compare
…ed declarations from one checker.Env to derivations created using Extend
633712c
to
d3ca467
Compare
There was a problem hiding this 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.
@@ -432,6 +462,16 @@ func (e *Env) configure(opts []EnvOption) (*Env, error) { | |||
if err != nil { | |||
return nil, err | |||
} | |||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
withininternalIsAssignable
to only the cases where recursive type substitutionsoccur. 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
OptionWhen 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 thecel.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 validateddeclarations into the cloned
checker.Env
of the extended environment. The cloned declarations areshallowly copied on
checker.Env
initialization. If a declaration provided to the derived environmentneeds 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 whosedeclarations have been validated as part of an
init()
call. This means in the common case that whenonly 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 thechecker.Env
. Thisbehavior 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 betweenExtend
calls.
Performance
The performance impact of these changes is dramatic, dropping the cost of the simple case by >95%