Skip to content

Conversation

@sairamch04
Copy link
Contributor

@sairamch04 sairamch04 commented Feb 7, 2023

Why ?

The current Scope Key is defined by creating a new string based on the metric tags and prefixes. This is inefficient as it ends up creating temporary strings for each metric emission. In our production service, the memory allocations due to scope key construction contribute to ~10% of the entire service allocations. This puts pressure on GC cycles, impacting the latencies of our service.

What ?

Instead of defining the key based on a temporary string(StringBuilder inside ScopeImpl.keyForPrefixedStringMap), we define it on already present objects. This will reduce unnecessary string allocations.

We create a new POJO called ScopeKey which wraps the existing tags and prefix objects, which will be the key for HashMap. HashMap internally uses hashcode() and equal() methods to determine the key. The memory of temporary string outweighs the memory of ScopeKey object as the latter has just the references to existing objects.

In short, We're changing the implementation of ScopeImpl.keyForPrefixedStringMap as part of this diff.

Benchmark results

Summary

  • Throughput increased from 1099 to 2306 Ops/Sec (~109% improvement)
  • Overall Memory allocation decreased from 1780 to 880 B/op (~50% improvement)
  • Old Gen churn decreased from 0.24 to 0.115 B/op (~50% improvement)

Before

image

After

Screenshot 2023-02-08 at 12 37 49 AM

@CLAassistant
Copy link

CLAassistant commented Feb 7, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@cdvr1993 cdvr1993 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@SokolAndrey SokolAndrey left a comment

Choose a reason for hiding this comment

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

we also keep the file with other benchmarks here, since you're changing the core part of the library could you please re-run those benchmarks and see if you change affects those

./gradlew runJmhTests

@sairamch04
Copy link
Contributor Author

@SokolAndrey : Thanks for the review. Ran all the benchmarks in tally-core and committed it. Please take a look again.

Copy link
Collaborator

@SokolAndrey SokolAndrey left a comment

Choose a reason for hiding this comment

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

lgtm! looking forward to see it in production!

@SokolAndrey SokolAndrey merged commit fa530e2 into uber-java:master Feb 13, 2023
@sairamch04 sairamch04 deleted the change-hashkey-tags branch February 13, 2023 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants