Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add concurrency to Exponential Histogram #5749

Open
wants to merge 3 commits into
base: 1.14.x
Choose a base branch
from
Open

Add concurrency to Exponential Histogram #5749

wants to merge 3 commits into from

Conversation

lenin-jaganathan
Copy link
Contributor

@lenin-jaganathan lenin-jaganathan commented Dec 18, 2024 •
edited
Loading

Guard the entire write operation externally for exponential histogram.

TODO:

  • Add concurrency tests (I will try to come to this next week)

Fixes #5740

@lenin-jaganathan lenin-jaganathan changed the title Defer to rescaling if values don't fit in the bucket Defer to rescaling ExponentialHistogram if values don't fit in the bucket Dec 18, 2024
@lenin-jaganathan lenin-jaganathan force-pushed the 5740_IOBE branch 2 times, most recently from 1029615 to d67f3e3 Compare December 20, 2024 07:48
@lenin-jaganathan lenin-jaganathan changed the title Defer to rescaling ExponentialHistogram if values don't fit in the bucket Add concurrency to Exponential Histogram Dec 20, 2024
@lenin-jaganathan lenin-jaganathan changed the base branch from main to 1.14.x December 20, 2024 07:53
@lenin-jaganathan
Add concurrency to exponential histogram
72cb6b1 
- Rescaling is not frequent behaviour (for cumulative scale should normalize after initial few recording, for delta it could increase/decrease but would be mostly stable). Instead of opting for a full synchronization on writes (which would have less performance), the choice of ReadWriteLock is chosen.
@lenin-jaganathan lenin-jaganathan mentioned this pull request Dec 20, 2024
Open
@lenin-jaganathan
Copy link
Contributor Author

I initially thought that using a ReadWriteLock would provide a better performance than normal synchronization. But, after running the benchmarks, that is not the case. The overhead of ReadLock seems to be higher compared to the simpler math operations (like incrementing values) that are performed inside the lock. Considering the code complexity it brings it seems that not a good option. I am sharing benchmark results from my local PC (M1 10 cores),

Existing Implementation 
1 - thread
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  96.115          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  85.981          ns/op

2 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  80.613          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  60.498          ns/op

4 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  211.090          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  179.947          ns/op

8 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  468.598          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  281.201          ns/op
Full Synchronization (the current state of PR) 
1 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  96.945          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  85.520          ns/op

2 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  424.293          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  324.563          ns/op

4 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  922.629          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  853.906          ns/op

8 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  1832.407          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  1662.399          ns/op
Full Synchronization with AtomicLongArray (we have been using a AtomicLongArray until now) 
1 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  94.078          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  84.147          ns/op

2 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  428.927          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  336.852          ns/op

4 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  865.197          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  817.838          ns/op

8 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  1708.773          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  1622.357          ns/op
ReadWrite Locks (Implementation in the first commit of this PR) 
1 - thread
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  107.122          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2   90.638          ns/op

2 - threads
Benchmark                                                     
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  481.020          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  261.227          ns/op

4 - threads
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  946.514          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  888.778          ns/op

8 - threads                                                
CompareOTLPHistograms.oltpCumulativeExponentialHistogramTimer  avgt    2  4375.336          ns/op
CompareOTLPHistograms.oltpDeltaExponentialHistogramTimer       avgt    2  4036.524          ns/op

I will leave the ReadWriteLock implementation in the commit history so if someone has a look at it, then can run the tests as well (or correct me if I am doing something wrong there). I add a second commit to drop those changes and add synchronization to the writes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Exponential histogram throws ArrayIndexOutOfBoundsException
1 participant
@lenin-jaganathan