-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Slow arithmetic operations in hot path #6021
Comments
Thanks for the write-up! Something like var prebuiltInts [256]big.Float
func init() {
for i := range prebuiltInts {
prebuiltInts[i] = ...
}
} in the right place, paired with the right code that'll return one of these if the number we're parsing is lower than 256, should do the trick for many cases, I think. |
Looks straightforward to me! I suppose for simple arithmetic of low-range numbers, like 22 - 1, we wouldn't really need to work with big floats at all? But perhaps it'd add too much clutter trying to account for when it's appropriate and when it isn't. |
Might be reasonable to measure along the way. If one trick has a significant-enough impact, we could leave it at that. But in general, yeah, small numbers should just stay small 😄 |
https://gist.github.com/srenatus/5361bfa8c4044aad97fa62af27d5bbcb |
Currently for arithmetic operations like addition, subtraction etc. the operands are first converted to the big float type before operating on them. This conversion can be avoided for small numbers w/o impacting the accuracy of the result. This change attemtps to avoid the conversion for small numbers by caching them and thereby helping to speedup some operations. Fixes: open-policy-agent#6021 Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Currently for arithmetic operations like addition, subtraction etc. the operands are first converted to the big float type before operating on them. This conversion can be avoided for small numbers w/o impacting the accuracy of the result. This change attemtps to avoid the conversion for small numbers by caching them and thereby helping to speedup some operations. Fixes: open-policy-agent#6021 Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Currently for arithmetic operations like addition, subtraction etc. the operands are first converted to the big float type before operating on them. This conversion can be avoided for small numbers w/o impacting the accuracy of the result. This change attemtps to avoid the conversion for small numbers by caching them and thereby helping to speedup some operations. Fixes: open-policy-agent#6021 Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
Or dealing with numbers in general. While it's admittedly not a case one likely will be bothered by in what could be considered normal policy evaluation, Rego is everywhere these days, and its use cases sometimes unorthodox. One such case would be Regal, where we traverse large abstract syntax trees (using the
walk
built-in function) in order to find bugs and inefficiencies in Rego code. Doing so means we'll sometimes traverse hundreds of thousands of nodes, and in most cases we only look closer at a node if the last element of the path is of interest. To do so, we do a simple lookup of the last element:When benchmarking Regal on some huge policy libraries, this seemingly simple operation accounted for roughly 20% of the total evaluation time.
I believe this is caused by the frequent creation of big floats for the
count
, the operands (ie.new(big.Float).SetString(string(n))
) and of course the subtraction itself (new(big.Float).Sub(a, b)
).Creating 600'000 big floats for the constant number
1
seems excessive, and I believe we could be more efficient by e.g. caching the most common numbers (like any up to 256 or whatnot), or even recognize the "last element construct" and optimize for that. But there might be better optimizations to improve the efficiency of working with numbers across the board. Thenumbers.range
function comes to mind here too, as it performs poorly when creating a large amount of numbers. Again, this is not a common use case, but if we can improve the uncommon ones, perhaps they'll be more common one day :)For now we've solved this with a custom regal.last built-in, which has the benefit of knowing it's job is a last element lookup, and the result is a 20% reduction of eval time:
path[count(path) - 1]
regal.last
While this works well, we'd ideally deal with this better in OPA proper.
The text was updated successfully, but these errors were encountered: