Nested metric registries don't cause a sortedmap creation #430

j-baker · 2019-09-18T10:39:06Z

This might seem like an unnecessary optimization, but profiling
internally implies this uses plenty of CPU and allocates gigabytes and
gigabytes and gigabytes (about 1.8GB of allocations to report metrics).

BLUF: With nested tags, our tooling spends all of its time recreating
nested sorted maps and rehashing, which is very expensive. That said,
breaking binary backcompat here is super expensive, as we know from past
experience.

This PR solves this issue. We no longer use the immutables generated
class, though we do use the builder. Instead, we use our own
implementation, which hashes consistently and uses two map
implementations to ensure that in the cases we see, equality, hash, and
extending with another tag are cheap.

This might seem like an unnecessary optimization, but profiling internally implies this uses plenty of CPU and allocates gigabytes and gigabytes and gigabytes (about 1.8GB of allocations to report metrics). BLUF: With nested tags, our tooling spends all of its time recreating nested sorted maps and rehashing, which is very expensive. That said, breaking binary backcompat here is super expensive, as we know from past experience. This PR solves this issue. We no longer use the immutables generated class, though we do use the builder. Instead, we use our own implementation, which hashes consistently and uses two map implementations to ensure that in the cases we see, equality, hash, and extending with another tag are cheap.

changelog-app · 2019-09-18T10:39:11Z

Generate changelog in `changelog/@unreleased`

Type

Description

Nested metric registries cause fewer allocations.

Check the box to generate changelog(s)

Generate changelog entry

carterkozak · 2019-09-18T10:48:52Z

Would you mind adding a representative JMH benchmark?

j-baker · 2019-09-18T10:52:45Z

That seems like a great idea.

CRogers · 2019-09-18T10:53:11Z

Chatted to @j-baker on slack, would be good to see a profile on this (possibly in prod) before we merge this as it's used by every product.

carterkozak · 2019-09-18T10:54:43Z

Could we do something with a TaggedMetricRegistry.forEach(BiConsumer<MetricName, Metric>) instead?

j-baker · 2019-09-18T11:30:35Z

I ended up doing both (and came to the forEach conclusion as well). Here are benchmark results before and after:

Before                                                                    Mode  Cnt        Score         Error   Units
NestedMetricsBenchmark.benchmarkForEach                                      avgt   10   949386.155 ±  290036.990   ns/op
NestedMetricsBenchmark.benchmarkForEach:·gc.alloc.rate.norm                  avgt   10  2122032.414 ±       0.121    B/op
NestedMetricsBenchmark.benchmarkGetMetrics                                   avgt   10  1627004.644 ±  361734.975   ns/op
NestedMetricsBenchmark.benchmarkGetMetrics:·gc.alloc.rate.norm               avgt   10  2800648.214 ±      12.257    B/op

After                                                                    Mode  Cnt        Score        Error   Units
NestedMetricsBenchmark.benchmarkForEach                                      avgt   10   214789.191 ±  66852.954   ns/op
NestedMetricsBenchmark.benchmarkForEach:·gc.alloc.rate.norm                  avgt   10   322032.094 ±      0.028    B/op
NestedMetricsBenchmark.benchmarkGetMetrics                                   avgt   10   557286.570 ±  78861.278   ns/op
NestedMetricsBenchmark.benchmarkGetMetrics:·gc.alloc.rate.norm               avgt   10   997784.244 ±      0.039    B/op

as you can see, even in the case where we only have a single pre-existing tag, this is a considerable improvement.

j-baker · 2019-09-18T11:32:33Z

This is also only to write 5k metrics - in practice we see many more.

carterkozak · 2019-09-18T12:37:44Z

I think we should update the benchmark into a root TaggedMetricRegistry with N tagged metrics, a child wrapped DropwizardMetricSet(MetricRegistry) with N metrics, and a child TaggedMetricRegistry with N metrics (representing SharedTaggedMetricRegistries.getSingleton()).
That should scale in a way that represents production systems.

I'd prefer to put all our eggs in the forEachMetric basket and avoid optimizations on getMetrics to reduce complexity. If we provide a simple MetricName implementation which doesn't pre-compute hashCode, and avoid precomputing hashcode on ExtraEntrySortedMap, we should have strictly less work.

Might be worth special casing adding a single tag to a MetricName with no tags, and adding a simple MetricName implementation around a safeName with no tags for DropwizardTaggedMetricSet.

…nto jbaker/faster_strings

CRogers · 2019-09-18T12:41:28Z

...um-registry/src/test/java/com/palantir/tritium/metrics/registry/ExtraEntrySortedMapTest.java

+import net.jqwik.api.constraints.IntRange;
+import net.jqwik.api.constraints.Size;
+
+class ExtraEntrySortedMapTest {


I've added this property based test as I wanted to make sure the behaviour of ExtraEntrySortedMap was correct but also not write a ton of example based tests. This checks that every method call on ExtraEntrySortedMap produces the same result as the same method call on an equivalent guava ImmutableSortedMap, for thousands of arbitrary base maps and extra values. It uses jqwik.

schlosna · 2019-09-18T12:30:03Z

tritium-registry/src/main/java/com/palantir/tritium/metrics/registry/PrehashedSortedMap.java

+    private final SortedMap<K, V> delegate;
+    private final int hashCode;
+
+    PrehashedSortedMap(SortedMap<K, V> delegate) {


would suggest making immutability explicit in types

Suggested change

PrehashedSortedMap(SortedMap<K, V> delegate) {

PrehashedSortedMap(ImmutableSortedMap<K, V> delegate) {

This would require casting in RealMetricName, which I'd like to avoid.

not sure I follow that as the only caller of this constructor is com.palantir.tritium.metrics.registry.RealMetricName#prehash which explicitly passes in an ImmutableSortedMap

return new PrehashedSortedMap<>(ImmutableSortedMap.copyOfSorted(map));

tritium-registry/src/main/java/com/palantir/tritium/metrics/registry/PrehashedSortedMap.java

tritium-registry/src/main/java/com/palantir/tritium/metrics/registry/ExtraEntrySortedMap.java

tritium-registry/src/main/java/com/palantir/tritium/metrics/registry/PrehashedSortedMap.java

schlosna · 2019-09-18T13:09:58Z

tritium-registry/src/main/java/com/palantir/tritium/metrics/registry/RealMetricName.java

+    private int computeHashCode() {
+        int hash = 5381;
+        hash += (hash << 5) + safeName().hashCode();
+        hash += (hash << 5) + safeTags().hashCode();


just curious why djb2 hash algorithm?

it's the one in the immutables generated code

copy-pasted :P

tritium-registry/src/main/java/com/palantir/tritium/metrics/registry/ExtraEntrySortedMap.java

Stricter null checks

j-baker · 2019-09-18T19:07:53Z

.baseline/checkstyle/checkstyle.xml

@@ -469,7 +469,7 @@
        <module name="NestedTryDepth"/> <!-- Java Coding Guide: Try/catch blocks: never nested -->
        <module name="NonEmptyAtclauseDescription"/> <!-- Java Style Guide: At-clauses -->
        <module name="ParameterName"> <!-- Java Style Guide: Parameter names -->
-            <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
+            <property name="format" value="^_?[a-z][a-zA-Z0-9]+$"/>


as updated by baseline

assume we will also pick this up next time the robots do their rounds in https://github.com/palantir/gradle-baseline/releases/tag/2.7.0 though I don't see a tritium excavator listed in palantir/gradle-baseline#833

schlosna

LGTM 👍 , thanks James! Defer to you all when you want to merge & cut a release

schlosna · 2019-09-18T19:19:00Z

.baseline/checkstyle/checkstyle.xml

@@ -469,7 +469,7 @@
        <module name="NestedTryDepth"/> <!-- Java Coding Guide: Try/catch blocks: never nested -->
        <module name="NonEmptyAtclauseDescription"/> <!-- Java Style Guide: At-clauses -->
        <module name="ParameterName"> <!-- Java Style Guide: Parameter names -->
-            <property name="format" value="^[a-z][a-zA-Z0-9]+$"/>
+            <property name="format" value="^_?[a-z][a-zA-Z0-9]+$"/>


assume we will also pick this up next time the robots do their rounds in https://github.com/palantir/gradle-baseline/releases/tag/2.7.0 though I don't see a tritium excavator listed in palantir/gradle-baseline#833

svc-autorelease · 2019-09-18T19:30:35Z

Released 0.15.6

j-baker requested a review from schlosna as a code owner September 18, 2019 10:39

j-baker added 2 commits September 18, 2019 11:42

variable names

31021df

Add generated changelog entries

6c114f2

return false

672cc35

j-baker added 6 commits September 18, 2019 11:58

Small refactors

a2333e0

Benchmark

c8a2198

Add a forEach method on metrics

27c3686

Foreach optimized

161a4ed

Add gc profiler

8a352e5

Actually have another tag

c17f181

j-baker added 3 commits September 18, 2019 13:15

errorprone

391b6dd

extra constructor

4843289

Deal with painful thing

f8295fd

CRogers added 2 commits September 18, 2019 13:38

Add property based test for ExtraEntrySortedMap

bea4cb9

Merge branch 'jbaker/faster_strings' of github.com:palantir/tritium i…

247cf99

…nto jbaker/faster_strings

CRogers reviewed Sep 18, 2019

View reviewed changes

schlosna reviewed Sep 18, 2019

View reviewed changes

schlosna mentioned this pull request Sep 18, 2019

Stricter null checks #431

Merged

CRogers reviewed Sep 18, 2019

View reviewed changes

tritium-registry/src/main/java/com/palantir/tritium/metrics/registry/ExtraEntrySortedMap.java Outdated Show resolved Hide resolved

j-baker added 2 commits September 18, 2019 14:46

PR comments

dfa341f

More PR comments

b9db5cb

schlosna and others added 7 commits September 18, 2019 09:59

Stricter null checks

75fbcb0

Suppress JDK Obsolete for SortedMap test

5f5f61c

Fix copyright header

d8390ff

write locks

31dc183

perf regression

b26ad69

Merge pull request #431 from palantir/ds/faster-strings-cr

bfe2efd

Stricter null checks

Benchmark

969f8c0

j-baker commented Sep 18, 2019

View reviewed changes

Merge branch 'develop' into jbaker/faster_strings

90456c8

j-baker force-pushed the jbaker/faster_strings branch from 1c39f80 to 90456c8 Compare September 18, 2019 19:09

baseline...

2b014f0

schlosna approved these changes Sep 18, 2019

View reviewed changes

schlosna requested a review from carterkozak September 18, 2019 19:20

j-baker added autorelease merge when ready labels Sep 18, 2019

j-baker merged commit 980cac9 into develop Sep 18, 2019

j-baker deleted the jbaker/faster_strings branch September 18, 2019 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested metric registries don't cause a sortedmap creation #430

Nested metric registries don't cause a sortedmap creation #430

j-baker commented Sep 18, 2019

changelog-app bot commented Sep 18, 2019 •

edited by j-baker

Loading

carterkozak commented Sep 18, 2019

j-baker commented Sep 18, 2019

CRogers commented Sep 18, 2019

carterkozak commented Sep 18, 2019

j-baker commented Sep 18, 2019

j-baker commented Sep 18, 2019

carterkozak commented Sep 18, 2019

CRogers Sep 18, 2019 •

edited

Loading

schlosna Sep 18, 2019

j-baker Sep 18, 2019 •

edited

Loading

schlosna Sep 18, 2019

j-baker Sep 18, 2019

schlosna Sep 18, 2019

j-baker Sep 18, 2019

j-baker Sep 18, 2019

j-baker Sep 18, 2019

schlosna Sep 18, 2019

schlosna left a comment

schlosna Sep 18, 2019

svc-autorelease commented Sep 18, 2019

	PrehashedSortedMap(SortedMap<K, V> delegate) {
	PrehashedSortedMap(ImmutableSortedMap<K, V> delegate) {

Nested metric registries don't cause a sortedmap creation #430

Nested metric registries don't cause a sortedmap creation #430

Conversation

j-baker commented Sep 18, 2019

changelog-app bot commented Sep 18, 2019 • edited by j-baker Loading

Generate changelog in changelog/@unreleased

carterkozak commented Sep 18, 2019

j-baker commented Sep 18, 2019

CRogers commented Sep 18, 2019

carterkozak commented Sep 18, 2019

j-baker commented Sep 18, 2019

j-baker commented Sep 18, 2019

carterkozak commented Sep 18, 2019

CRogers Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-baker Sep 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlosna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svc-autorelease commented Sep 18, 2019

changelog-app bot commented Sep 18, 2019 •

edited by j-baker

Loading

Generate changelog in `changelog/@unreleased`

CRogers Sep 18, 2019 •

edited

Loading

j-baker Sep 18, 2019 •

edited

Loading