You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2023/01/05 15:43:13 UTC
[GitHub] [beam] damccorm commented on a diff in pull request #24881: Reduce lock contention for metrics hashName hasher
damccorm commented on code in PR #24881:
URL: https://github.com/apache/beam/pull/24881#discussion_r1062615527
##########
sdks/go/pkg/beam/core/metrics/metrics_test.go:
##########
@@ -617,21 +617,23 @@ func TestPcolQueryResult(t *testing.T) {
}
}
-// Run on @lostluck's desktop (2020/01/21) go1.13.4
+// Run on @shanemhansen's desktop (2022/01/03) go1.20 RC1 after changing hashName to use a pool of hashers
+// sync.Pool can return thread-local results eliminating the need for a lock and increasing throughput.
+// There are users in the wild who create an excessive number of Counters so a 4x improvement in throughput at the expense of
+// creating GOMAXPROCS hasher values seems reasonable.
//
-// Allocs & bytes should be consistent within go versions, but ns/op is relative to the running machine.
-//
-// BenchmarkMetrics/counter_inplace-12 6054129 208 ns/op 48 B/op 1 allocs/op
-// BenchmarkMetrics/distribution_inplace-12 5707147 228 ns/op 48 B/op 1 allocs/op
-// BenchmarkMetrics/gauge_inplace-12 4742331 259 ns/op 48 B/op 1 allocs/op
-// BenchmarkMetrics/counter_predeclared-12 90147133 12.7 ns/op 0 B/op 0 allocs/op
-// BenchmarkMetrics/distribution_predeclared-12 55396678 21.6 ns/op 0 B/op 0 allocs/op
-// BenchmarkMetrics/gauge_predeclared-12 18535839 60.5 ns/op 0 B/op 0 allocs/op
-// BenchmarkMetrics/counter_raw-12 159581343 7.18 ns/op 0 B/op 0 allocs/op
-// BenchmarkMetrics/distribution_raw-12 82724314 14.7 ns/op 0 B/op 0 allocs/op
-// BenchmarkMetrics/gauge_raw-12 23292386 55.2 ns/op 0 B/op 0 allocs/op
-// BenchmarkMetrics/getStore-12 309361303 3.78 ns/op 0 B/op 0 allocs/op
-// BenchmarkMetrics/getCounterSet-12 287720998 3.98 ns/op 0 B/op 0 allocs/op
+// name old time/op new time/op delta
+// Metrics/counter_inplace-12 376ns ±17% 88ns ± 7% -76.66% (p=0.008 n=5+5)
+// Metrics/distribution_inplace-12 394ns ± 3% 153ns ± 8% -61.17% (p=0.008 n=5+5)
+// Metrics/gauge_inplace-12 371ns ± 4% 258ns ± 1% -30.37% (p=0.008 n=5+5)
Review Comment:
Nice
##########
sdks/go/pkg/beam/core/metrics/metrics_test.go:
##########
@@ -617,21 +617,23 @@ func TestPcolQueryResult(t *testing.T) {
}
}
-// Run on @lostluck's desktop (2020/01/21) go1.13.4
+// Run on @shanemhansen's desktop (2022/01/03) go1.20 RC1 after changing hashName to use a pool of hashers
+// sync.Pool can return thread-local results eliminating the need for a lock and increasing throughput.
+// There are users in the wild who create an excessive number of Counters so a 4x improvement in throughput at the expense of
+// creating GOMAXPROCS hasher values seems reasonable.
Review Comment:
We should probably cut some of this before merging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org