avoid repeated construction of cost tracker#1357
Conversation
|
/gcbrun |
|
@pohly Thanks for this change. Would you mind adding a test case that initiates initiates the cost tracking against a single compiled expression in a short loop to help prevent future regressions? |
|
How can such a test verify that Eval doesn't call NewCostTracker again? Just calling Eval multiple times will not fail, with or without this patch. |
|
It's purely defensive ... more that if something changes in the cost tracker in the future to make sure we don't miss the updates to |
|
A single Eval always uses NewCostTracker (during program construction) followed by Clone (when the factory function is called). But it doesn't hurt to do Eval more than once to cover the scenario that Clone is called more than once - will add something. |
Most fields in the CostTracker struct are only set once during construction. This takes a surprising amount of allocations, in particular for the overload map. Constructing the tracker once and cloning it for each evaluation avoids doing that work repeatedly. In a Kubernetes benchmark exercising (among other things) CEL expression evaluation, the cumulative CPU time and allocs for Eval dropped from 3.87s to 1.99s resp. from 3.85G to 2.1G.
pohly
force-pushed
the
cost-tracker-construction
branch
from
c0b1db8 to
164e53d
Compare
|
@TristonianJones: see https://github.com/cel-expr/cel-go/compare/c0b1db8e505326cc8463b8fed381c116a5ff9de1..164e53d3c0baad4da72f0df5400ef1526ffcc6a7 for unit tests.
|
|
@TristonianJones: gentle ping. Is there a chance to get this into a tagged release soonish? Code freeze deadline for Kubernetes is July 22nd, it would be great to bump cel-go there well in advance. |
|
/gcbrun |
There may be some useful enhancements in https://github.com/cel-expr/cel-go/releases/tag/v0.28.0, but primarily the goal is to state up-to-date to simplify updating to a release which includes cel-expr/cel-go#1357 because that is a relevant performance improvement for how DRA uses CEL. Updating to v0.28.1 is safe, in contrast to v0.28.0, because v0.28.1 includes cel-expr/cel-go#1303 and therefore doesn't trigger kubernetes#138334.
There may be some useful enhancements in https://github.com/cel-expr/cel-go/releases/tag/v0.28.0 and https://github.com/cel-expr/cel-go/releases/tag/v0.29.0, but primarily the goal is to get cel-expr/cel-go#1357 (included in v0.29.0) because that is a relevant performance improvement for how DRA uses CEL. Updating to >= v0.28.1 is safe, in contrast to v0.28.0, because v0.28.1 includes cel-expr/cel-go#1303 and therefore doesn't trigger kubernetes#138334.
2 participants
Most fields in the CostTracker struct are only set once during construction. This takes a surprising amount of allocations, in particular for the overload map. Constructing the tracker once and cloning it for each evaluation avoids doing that work repeatedly.
In a Kubernetes benchmark exercising (among other things) CEL expression evaluation, the cumulative CPU time and allocs for Eval dropped from 3.87s to 1.99s resp. from 3.85G to 2.1G.