Skip to content

Conversation

abi87
Copy link
Contributor

@abi87 abi87 commented Jul 24, 2024

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:

Benchmark_Dimensions_Add-12    	47075594	        25.24 ns/op	       0 B/op	       0 allocs/op
Benchmark_Dimensions_Sub-12    	55160683	        21.76 ns/op	       0 B/op	       0 allocs/op

Pointer receiver:

Benchmark_Dimensions_Add-12    	46841310	        25.22 ns/op	       0 B/op	       0 allocs/op
Benchmark_Dimensions_Sub-12    	55187002	        21.95 ns/op	       0 B/op	       0 allocs/op

How this works

Just added a couple of benchmarks

How this was tested

Benchmarks

@abi87 abi87 self-assigned this Jul 24, 2024
@abi87 abi87 requested a review from StephenButtolph as a code owner July 24, 2024 08:30
@abi87 abi87 requested review from ARR4N and tsachiherman July 24, 2024 08:30
Copy link
Contributor

@ARR4N ARR4N left a 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()
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@StephenButtolph
Copy link
Contributor

Benchmarks report zero allocations, which I understand means the compiler is able to optimize out copies.

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...)
Copy link
Contributor

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.

@abi87 abi87 requested a review from StephenButtolph July 25, 2024 12:29
@StephenButtolph
Copy link
Contributor

Was the purpose of this PR to benchmark whether to:

  • use a value receiver or a pointer receiver?
  • pass in pointers for the rhs?

If we change how the rhs is passed into the benchmark, I'd expect the pointer values to be faster

@abi87 abi87 force-pushed the fee_dimensions_add_benchmark branch from f5a0be5 to de23f74 Compare July 25, 2024 15:35
Copy link
Contributor

@StephenButtolph StephenButtolph left a 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

@StephenButtolph StephenButtolph added this to the v1.11.11 milestone Jul 26, 2024
@StephenButtolph StephenButtolph changed the title Added benchmarks for add and sub fee dimensions Add benchmarks for add and sub fee dimensions Jul 26, 2024
@StephenButtolph StephenButtolph added this pull request to the merge queue Jul 26, 2024
Merged via the queue into master with commit df27dfe Jul 26, 2024
@StephenButtolph StephenButtolph deleted the fee_dimensions_add_benchmark branch July 26, 2024 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants