-
Notifications
You must be signed in to change notification settings - Fork 807
Add benchmarks for add and sub fee dimensions #3222
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving with comments that I don't need to re-review if implemented.
_, err := lhs.Add(rhs...) | ||
require.NoError(err) | ||
} | ||
b.StopTimer() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need to explicitly call StopTimer()
at the end. It's for pausing (and later resuming) in the middle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
That is not what the allocations are reporting in the benchmarks. Allocations are reporting heap allocations. Not stack copies. |
|
||
b.ResetTimer() | ||
for i := 0; i < b.N; i++ { | ||
_, err := lhs.Add(rhs...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By passing in varargs with a slice expansion, you are not copying the dimensions. The slice is passed in directly without copying.
Was the purpose of this PR to benchmark whether to:
If we change how the rhs is passed into the benchmark, I'd expect the pointer values to be faster |
f5a0be5
to
de23f74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely not going to impact performance, but might as well save ~6%.
goos: darwin
goarch: arm64
pkg: github.com/ava-labs/avalanchego/vms/components/fee
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
_Dimensions_Add/single-12 4.469n ± 1% 4.146n ± 3% -7.22% (p=0.000 n=10)
_Dimensions_Add/multiple-12 28.12n ± 1% 26.57n ± 5% -5.49% (p=0.001 n=10)
_Dimensions_Sub/single-12 4.465n ± 0% 4.130n ± 3% -7.48% (p=0.001 n=10)
_Dimensions_Sub/multiple-12 25.47n ± 1% 23.03n ± 6% -9.60% (p=0.000 n=10)
geomean 10.93n 10.12n -7.46%
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
_Dimensions_Add/single-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
_Dimensions_Add/multiple-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
_Dimensions_Sub/single-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
_Dimensions_Sub/multiple-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
geomean ² +0.00% ²
¹ all samples are equal
² summaries must be >0 to compute geomean
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
_Dimensions_Add/single-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
_Dimensions_Add/multiple-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
_Dimensions_Sub/single-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
_Dimensions_Sub/multiple-12 0.000 ± 0% 0.000 ± 0% ~ (p=1.000 n=10) ¹
geomean ² +0.00% ²
¹ all samples are equal
² summaries must be >0 to compute geomean
Why this should be merged
Dimensions are structured as value objects meaning that Add and Sub methods have a value receiver.
This causes receiver to be copied, but the receiver itself can be reused as the returned value.
On the contrary, if we used a pointer received, we'd avoid copying the receiver but we'd have to allocate the return value.
This PR introduces a couple of small benchmarks to understand what's the fastest alternative.
There seems not to be a significant different among the two alternative
Value receiver:
Pointer receiver:
How this works
Just added a couple of benchmarks
How this was tested
Benchmarks