execution: reorganize function operators #285
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MichaHoffmann
force-pushed
the
mhoffm-reorganize-function-operators
branch
4 times, most recently
from
3108c8a
to
48cf5cb
Compare
fpetkovski
reviewed
Jun 8, 2023
| StepTime int64 | ||
| SelectRange int64 | ||
| ScalarPoints []float64 | ||
| Offset int64 |
There was a problem hiding this comment.
Is there a way to pass Offset and SelectRange via a clojure when creating the function? These values should be constant and do not change between steps, so we might get a small speed up if there are less arguments to put on the stack before the function call.
The arguments are also optional for most functions so there might be an unnecessary overhead for something like sum_over_time.
execution/scan/functions.go
Outdated
| }, | ||
| "xincrease": func(f functionArgs) sample { | ||
| if len(f.Samples) == 0 { | ||
| return invalidSample |
There was a problem hiding this comment.
I wonder if we should remove invalidSample in favour of an ok return argument to indicate whether the function returns any value. The return args would then be float64, *histogram.FloatHistogram, bool. We also don't need the timestamp of the result sample since that comes from the step itself.
There was a problem hiding this comment.
that sounds much better! thanks ill do that
There was a problem hiding this comment.
done! it has some effect
on main:
RangeQuery/clamp-8 632.5m ± ∞ ¹ 102.5m ± ∞ ¹ -83.79% (p=0.008 n=5)
vs PR
RangeQuery/clamp-8 631.62m ± ∞ ¹ 85.65m ± ∞ ¹ -86.44% (p=0.008 n=5)
execution/function/functions.go
Outdated
| ) | ||
|
|
||
| var InvalidSample = Sample{T: -1, F: 0} | ||
| var invalidSample = sample{T: -1, F: 0} |
There was a problem hiding this comment.
Here I would also consider removing invalidSample in favour of 3 return arguments and dropping the timestamp.
execution/function/functions.go
Outdated
| MetricAppearedTs *int64 | ||
| type functionArgs struct { | ||
| Samples []sample | ||
| StepTime int64 |
There was a problem hiding this comment.
Do we need to use this parameter in functions?
There was a problem hiding this comment.
ah no, we only need it in range functions i think, not removing it was an oversight
There was a problem hiding this comment.
ah its somewhat used in the time related functions; most of the time not though
There was a problem hiding this comment.
i propose that we keep it for now and maybe handle the time based functions with a special sub operator here, wdyt?
There was a problem hiding this comment.
i went ahead and removed it too because it would complicate the date functions. i now introduced noArgs and withArgs variants of them and got rid of step time most of the functions that way.
MichaHoffmann
force-pushed
the
mhoffm-reorganize-function-operators
branch
6 times, most recently
from
2a9c4ca
to
e39a8da
Compare
Signed-off-by: Michael Hoffmann <[email protected]>
Signed-off-by: Michael Hoffmann <[email protected]>
Signed-off-by: Michael Hoffmann <[email protected]>
MichaHoffmann
force-pushed
the
mhoffm-reorganize-function-operators
branch
from
e39a8da
to
1750741
Compare
Signed-off-by: Filip Petkovski <[email protected]>
fpetkovski
force-pushed
the
mhoffm-reorganize-function-operators
branch
from
9db0431
to
270c534
Compare
fpetkovski
approved these changes
Jul 7, 2023
Signed-off-by: Filip Petkovski <[email protected]>
fpetkovski
force-pushed
the
mhoffm-reorganize-function-operators
branch
from
c2ae5b7
to
ef8e932
Compare
|
Benchmark results against main |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
scandoes not depend onfunctionanymore we can hide a couple structs and functions