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.

Sign up for GitHub

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

Jump to bottom

Runtime cost calculation #494

Merged
merged 5 commits into from
Mar 8, 2022
Merged

Runtime cost calculation #494

merged 5 commits into from
Mar 8, 2022

Conversation

cici37
Copy link
Collaborator

@cici37 cici37 commented Feb 23, 2022 •
edited
Loading

Introduce runtime cost calculation into CEL.

The work is part of this issue

The current PR includes:

  • Add runtime cost calculation into evaluation path
  • Test to verify runtime cost calculation
  • Test to verify runtime cost fell into estimation cost range
  • Test to allow cost calculation for extension function

Todo in following PR:

  • Add cancellation
  • Add more test for large input
  • Add test coverage for aligning runtime cost with estimate cost

jpbetz
jpbetz requested changes Feb 23, 2022
View reviewed changes
cel/program.go Outdated Show resolved Hide resolved
cel/cel_test.go Outdated Show resolved Hide resolved
cel/options.go Show resolved Hide resolved
cel/options.go Outdated Show resolved Hide resolved
cel/program.go Show resolved Hide resolved
interpreter/runtimecost.go Show resolved Hide resolved
interpreter/runtimecost.go Outdated Show resolved Hide resolved
interpreter/runtimecost.go Outdated Show resolved Hide resolved
interpreter/runtimecost.go Outdated Show resolved Hide resolved
interpreter/runtimecost.go Show resolved Hide resolved
@jpbetz jpbetz mentioned this pull request Feb 23, 2022
Closed
15 tasks
@jpbetz
Copy link
Collaborator

jpbetz commented Feb 24, 2022

@cici37 Add any newly introduce go files to to the bazel files to appease cel-go-builder.

@cici37 cici37 force-pushed the runtime-cost branch 2 times, most recently from 042fa98 to fcfb47d Compare February 24, 2022 05:08
@cici37
Copy link
Collaborator Author

cici37 commented Feb 24, 2022

Cici Huang Add any newly introduce go files to to the bazel files to appease cel-go-builder.

Looks like I don't have permission to check cel-go-builder job detail. Is there any verification/script which I could use for checking?

jpbetz
jpbetz reviewed Feb 24, 2022
View reviewed changes
interpreter/runtimecost.go Outdated Show resolved Hide resolved
jpbetz
jpbetz reviewed Feb 24, 2022
View reviewed changes
cel/options.go Outdated Show resolved Hide resolved
jpbetz
jpbetz reviewed Feb 24, 2022
View reviewed changes
interpreter/runtimecost.go Outdated Show resolved Hide resolved
jpbetz
jpbetz reviewed Feb 24, 2022
View reviewed changes
cel/options.go Outdated Show resolved Hide resolved
jpbetz
jpbetz reviewed Feb 24, 2022
View reviewed changes
cel/options.go Outdated Show resolved Hide resolved
@jpbetz
Copy link
Collaborator

jpbetz commented Feb 26, 2022

This looks good to me. As future work I'd like to start reducing the number of decorators we will need, but I see that as an optimization.

@TristonianJones would you be willing to do a review pass?

@cici37 cici37 marked this pull request as ready for review February 28, 2022 22:14
@cici37 cici37 mentioned this pull request Mar 1, 2022
Closed
TristonianJones
TristonianJones reviewed Mar 1, 2022
View reviewed changes
interpreter/attributes.go Outdated Show resolved Hide resolved
interpreter/attributes.go Outdated Show resolved Hide resolved
interpreter/attributes.go Outdated Show resolved Hide resolved
interpreter/interpretable.go Outdated Show resolved Hide resolved
interpreter/attributes.go Outdated Show resolved Hide resolved
@jpbetz jpbetz force-pushed the runtime-cost branch 2 times, most recently from 3245af6 to 0819e6f Compare March 3, 2022 00:45
@cici37 cici37 mentioned this pull request Mar 3, 2022
Merged
TristonianJones
TristonianJones requested changes Mar 4, 2022
View reviewed changes
interpreter/interpreter.go Outdated Show resolved Hide resolved
cel/runtimecost_test.go Outdated Show resolved Hide resolved
interpreter/runtimecost.go Outdated Show resolved Hide resolved
interpreter/runtimecost.go Show resolved Hide resolved
cel/cel_test.go Outdated Show resolved Hide resolved
interpreter/runtimecost.go Outdated Show resolved Hide resolved
interpreter/interpreter.go Outdated Show resolved Hide resolved
interpreter/interpreter.go Show resolved Hide resolved
cel/runtimecost_test.go Outdated Show resolved Hide resolved
cel/runtimecost_test.go Outdated Show resolved Hide resolved
@jpbetz jpbetz force-pushed the runtime-cost branch 2 times, most recently from 6b564e4 to 002182a Compare March 5, 2022 00:33
@jpbetz
Copy link
Collaborator

jpbetz commented Mar 5, 2022

/gcbrun

@jpbetz
Copy link
Collaborator

jpbetz commented Mar 5, 2022

/gcbrun

1 similar comment
@TristonianJones
Copy link
Collaborator

/gcbrun

TristonianJones
TristonianJones reviewed Mar 7, 2022
View reviewed changes
Copy link
Collaborator

@TristonianJones TristonianJones left a comment

Choose a reason for hiding this comment

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

Almost there

interpreter/interpreter.go Outdated Show resolved Hide resolved
cel/options.go Show resolved Hide resolved
interpreter/interpreter.go Show resolved Hide resolved
interpreter/interpreter.go Show resolved Hide resolved
@jpbetz
Copy link
Collaborator

jpbetz commented Mar 7, 2022

/gcprun

@jpbetz
Copy link
Collaborator

jpbetz commented Mar 7, 2022

/gcbrun

TristonianJones
TristonianJones reviewed Mar 8, 2022
View reviewed changes
interpreter/interpreter.go

const (
// ContextCancelled indicates that the operation was cancelled in response to a Golang context cancellation.
ContextCancelled CancellationCause = iota
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is currently not used. Please do a follow up to plumb through the error or remove it from this context.

TristonianJones
TristonianJones reviewed Mar 8, 2022
View reviewed changes
interpreter/interpreter.go
}

// TODO: Replace all usages of ExhaustiveEval with ExhaustiveEvalWrapper
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can remove this TODO since it's been replaced.

@jpbetz jpbetz merged commit a22969d into google:master Mar 8, 2022
This was referenced Mar 8, 2022
Open
Merged
@jpbetz jpbetz mentioned this pull request Mar 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpbetz jpbetz jpbetz requested changes

@TristonianJones TristonianJones TristonianJones approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cici37 @jpbetz @TristonianJones