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

Runtime support for cel.@block #1048

Closed
wants to merge 6 commits into from

Conversation

TristonianJones
Copy link
Collaborator

Introduces runtimes support for cel.@block

CEL block is used by static optimizers to group subexpressions together
such that the following is true:

  • Each subexpression value may be referenced by its ordinal through the
    variable: @index<ordinal>
  • Subexpressions must be organized such that the expression at @index<N>
    may only depend on variables represented in @index<N-1>
  • Index expressions may not occur within a comprehension loop since each
    comprehension represents a scope which declares its own variables.

Depends on google/cel-spec#402 before submission

ext/bindings.go Outdated Show resolved Hide resolved
interpreter.Activation
slotExprs []interpreter.Interpretable
slotCount int
slotVals []*slotVal
Copy link
Collaborator

Choose a reason for hiding this comment

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

I personally would just avoid the pointer here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The pointer ends up being pretty convenient since it means that I can update the slotVal state without having to set the slot index multiple times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Any other concerns? I've written it both ways and the visit logic is easier to follow with the pointers. Object pooling generally solves any overhead concerns. See #1056

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wound up running tests against the pooled allocations versus stack-based allocs and overall the pointers with pooled memory were a 5% win, so I'm sticking with that for now and just merged the policy composition support. Happy to come back to this as a follow up if desired.

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.

2 participants