Add benchmark tests for StatsReport implementations by SokolAndrey · Pull Request #91 · uber-java/tally

SokolAndrey · 2021-01-31T11:54:43Z

Adds benchmark tests for all StatsReporter implementations.

andrewmains12

Lgtm in general, though definitely worth getting a pass from @alexeykudinkin since he has more Java perf experience than me.

andrewmains12 · 2021-02-01T16:54:50Z

+Benchmark Mode Cnt Score Error Units
+PrometheusReporterBenchmark.reportCounterBenchmark thrpt 10 243.828 ± 5.643 ops/ms
+PrometheusReporterBenchmark.reportGaugeBenchmark thrpt 10 247.080 ± 5.012 ops/ms
+PrometheusReporterBenchmark.reportHistogramDurationSamplesBenchmark thrpt 10 0.694 ± 0.114 ops/ms


Dang, thoughts on what's going on with this one? (obviously we could do the optimizations themselves in a separate PR).

Heavy memory churn

^ sounds plausible, looking forward to the optimizations there in the next pass.

this is the worst part of this prom implementation, which also mimics go-lang lib. The issue here is the difference between implementations of m3 and prom histograms. M3 uses counters to implement buckets and then reports each bucket separately. While prom updates all the buckets at the same time in a for-cycle.
public void reportHistogramDurationSamples( String name, Map<String, String> tags, Buckets buckets, Duration bucketLowerBound, Duration bucketUpperBound, long samples ) { ... for (int i = 0; i < samples; i++) { histogram.observe(bucketUpperBoundValue); } }
What for m3 is just a one-shot report for particular bucket, for prometheus is a for-cycle over all the buckets. So what we have here is something like a O(CN), where C is the number of samples, I used an incremental var from 0 to max long, and N is the number of buckets. I think using incremental value in range [0; MAX_LONG] is a bit of an overkill and not realistic. But there is definitely a huge room for improvements.

Also long samples is the delta for a bucket value between reporting intervals (which is configurable on client side).
I run the tests with an incremental value in range [0; 50] and the results are following:
Benchmark Mode Cnt Score Error Units PrometheusReporterBenchmark.reportCounterBenchmark thrpt 10 228.800 ± 39.789 ops/ms PrometheusReporterBenchmark.reportGaugeBenchmark thrpt 10 220.869 ± 22.208 ops/ms PrometheusReporterBenchmark.reportHistogramDurationSamplesBenchmark thrpt 10 222.604 ± 22.569 ops/ms PrometheusReporterBenchmark.reportHistogramValueSamplesBenchmark thrpt 10 209.727 ± 29.474 ops/ms PrometheusReporterBenchmark.reportTimerBenchmark thrpt 10 24.976 ± 4.750 ops/ms

@SokolAndrey this has been addressed in M3 fairly recently in #74. Prior to that it was also churning CPU traversing empty arrays.

#93 created an issue to address that.

andrewmains12 · 2021-02-01T19:51:38Z

@@ -0,0 +1,8 @@
+# JMH Benchmark Tests results
+
+Benchmark Mode Cnt Score Error Units


Btw @SokolAndrey do we have a sense of what the allocation profiles look like here? cc. @alexeykudinkin for approaches on getting that (I think Andrei mentioned utilizing the IntelliJ profiler there, which might work).

No need for external profilers, you can use JMH internal profilers for that.
https://stackoverflow.com/a/22642703

Ooh nice that's much better, thanks!

alexeykudinkin · 2021-02-02T02:32:14Z

Awesome work @SokolAndrey!

It would be great to add benchmark for the whole suite (Processor + Reporter) as well (we can do it separately if you prefer).

SokolAndrey · 2021-02-02T13:37:57Z

Awesome work @SokolAndrey!
It would be great to add benchmark for the whole suite (Processor + Reporter) as well (we can do it separately if you prefer).

Good idea! I guess it should fit in this PR

SokolAndrey · 2021-02-09T00:42:31Z

Awesome work @SokolAndrey!
It would be great to add benchmark for the whole suite (Processor + Reporter) as well (we can do it separately if you prefer).
Good idea! I guess it should fit in this PR

I guess it could be added as a separate PR. The issue is all the implementation of Reportable#report exit earlier if the reporting delta is 0. This makes benchmarking quite complicated because there must be something that triggers the metric change without affecting the measurement.

andrewmains12

Lgtm! Not sure if @alexeykudinkin has anything pending here; I'd say let's merge it if not.

Some followup things I'd want to see:

Possible benchmarks on adding new metric ids, not just incrementing old ones--users in general should optimize away from doing too much of that in hot paths, but it's not always avoidable, and we should make sure we don't die in that case.
Any optimizations, of course :) (I haven't read deeply through the results yet, but I expect there will be optimizations to be made).

alexeykudinkin

Left a few comments, but otherwise LGTM

alexeykudinkin · 2021-02-10T22:15:51Z

+PrometheusReporterBenchmark.reportHistogramDurationSamplesBenchmark:·gc.count thrpt 10 2.000 counts
+PrometheusReporterBenchmark.reportHistogramDurationSamplesBenchmark:·gc.time thrpt 10 4.000 ms
+PrometheusReporterBenchmark.reportHistogramValueSamplesBenchmark thrpt 10 0.777 ± 0.120 ops/ms
+PrometheusReporterBenchmark.reportHistogramValueSamplesBenchmark:·gc.alloc.rate thrpt 10 4.355 ± 0.653 MB/sec


Something is off in this benchmark:
It has substantially less alloc rate than counters (which is unlikely)
It has only 2 GCs occurring

i think it makes sense: the benchmark test has a long counter which goes up to Long.MAX_VALUE and with current implementation of histogram the tests keeps busy iterating over this huge long value and as a result there are not many allocations and not that many GCs happening. Need to fix this reportHistogram* methods

the tests keeps busy iterating over this huge long value and as a result there are not many allocations and not that many GCs happening
Not sure I get why we're iterating up to Long.MAX_VALUE in the test; are we testing the right thing here?

Sorry for the long reply. Need to configure notifications.
my reasoning there is:
we use long type as an input param => the value can potentially go up to the Long.MAX_VALUE (even though highly unlikely);
the value resets after each report interval, which is configurable on the client side. From what I found in the usages the report interval varies from 1s to few minutes;
Other implementations do not depend on the value range;
I couldn't reason for any other particular value as a threshold, so why not go up to the max;
It's a good indicator we need to improve it :)

Thanks for explanation!
Other implementations do not depend on the value range;
I didn't realize that this one did haha; definitely agreed that we should figure out how to not do that here. Will look more into that part of the implementation to develop a more informed opinion.

alexeykudinkin · 2021-02-10T22:17:28Z

@@ -0,0 +1,52 @@
+Benchmark Mode Cnt Score Error Units
+M3ReporterBenchmark.reportCounterBenchmark thrpt 10 392.408 ± 172.561 ops/ms


Let's also capture other benchmark values (other than GC related)

JMH only allows to run one profiler at a time. We can change the gradle task and pass different profilers as a param. But I'd prefer doing it incrementally in another PR

alexeykudinkin · 2021-02-10T22:19:07Z

@SokolAndrey this could be 2 separate tests:

With mostly static metric being reported (no updates)
With metric updated once per report tick (before the report we increment)

* Add benchmark tests for PrometheusReporter

Andrei Sokol added 3 commits January 31, 2021 11:56

Add benchmark tests for PrometheusReporter
651660f

Add benchmark tests for M3Reporter
b153234

Abstract ReporterBenchmark tests
808a13c

SokolAndrey commented Jan 31, 2021
View reviewed changes

Comment threadcore/src/main/java/com/uber/m3/jmh/AbstractReporterBenchmark.java Outdated

Andrei Sokol added 3 commits January 31, 2021 21:53

Add benchmark tests for StatsdReporter
923a6c3

Move AbstractReporterBenchmark to jmh module
14aae1d

Add tests results into README.md
1cf6397

SokolAndrey linked an issue Feb 1, 2021 that may be closed by this pull request
Add benchmark tests for all Reporter implementations #90
Closed

Fix core tests
07df1eb

andrewmains12 reviewed Feb 1, 2021
View reviewed changes

Comment threadcore/build.gradle

Andrei Sokol added 2 commits February 8, 2021 17:40

Add automatic reporting and docs for jmh test
7b7b2ff

Add gc profiling
ff6d012

SokolAndrey mentioned this pull request Feb 9, 2021
Improve PrometheusReporter performance #93
Open

andrewmains12 approved these changes Feb 9, 2021
View reviewed changes

alexeykudinkin reviewed Feb 10, 2021
View reviewed changes

SokolAndrey added 2 commits March 2, 2021 21:02

Merge branch 'master' into as/reporters_benchmarks
5aec8fc

Merge master
fa06099

andrewmains12 approved these changes Mar 10, 2021
View reviewed changes

SokolAndrey merged commit 5709ef5 into uber-java:masterMar 11, 2021

SokolAndrey deleted the as/reporters_benchmarks branch March 11, 2021 18:18

sairamch04 pushed a commit to sairamch04/tally that referenced this pull request Feb 5, 2023
Add benchmark tests for StatsReport implementations (uber-java#91)
5cd8a55
* Add benchmark tests for PrometheusReporter

		@@ -0,0 +1,8 @@
		# JMH Benchmark Tests results

		Benchmark Mode Cnt Score Error Units

		@@ -0,0 +1,52 @@
		Benchmark Mode Cnt Score Error Units
		M3ReporterBenchmark.reportCounterBenchmark thrpt 10 392.408 ± 172.561 ops/ms

Uh oh!

Conversation

SokolAndrey commented Jan 31, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

andrewmains12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewmains12Feb 2, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexeykudinkin commented Feb 2, 2021

Uh oh!

SokolAndrey commented Feb 2, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SokolAndrey commented Feb 9, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewmains12 left a comment

Choose a reason for hiding this comment

Uh oh!

alexeykudinkin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexeykudinkin commented Feb 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SokolAndrey commented Jan 31, 2021•
edited
Loading

andrewmains12Feb 2, 2021•
edited
Loading

SokolAndrey commented Feb 2, 2021•
edited
Loading

SokolAndrey commented Feb 9, 2021•
edited
Loading