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.

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

Memory leak in CKMSQuantiles.java #550

Closed
grksharma opened this issue May 11, 2020 · 6 comments
Closed

Memory leak in CKMSQuantiles.java #550

grksharma opened this issue May 11, 2020 · 6 comments

Comments

@grksharma
Copy link

This issue is more related to Memory leak in CKMSQuantiles.java? #422
I am getting this issue on a CKMSQuantiles$Quantile

@brian-brazil
Copy link
Contributor

Can you provide more details?

@grksharma
Copy link
Author

My application is running on Tomcat JVM. With increasing in requests/second to my application there is a memory leak in the JVM the memory is not getting released. Per my analysis the CollectorRegistry class usage causing the CKMSQuantiles$Quantile objects are not released.

The Heap dump in regular intervals show the leak suspect of 95% on the class CKMSQuantiles$Quantile.

@brian-brazil
Copy link
Contributor

Can you share graphs and code?

@grksharma grksharma reopened this May 11, 2020
@grksharma
Copy link
Author

Unfortunately I cannot share them as per the policies of my organization. But I observed it is pretty similar to the graphs of the issue #422

@brian-brazil
Copy link
Contributor

That wasn't an issue with client_java. I'm afraid without data, there's not much we can do.

@brian-brazil
Copy link
Contributor

Lacking information there's not much we can do here, and it's likely this code will be replaced at some point anyway.

DieBauer added a commit to DieBauer/client_java that referenced this issue Jan 22, 2022
CKMSQuantiles is copied from an implementation of 2012, where it states that a ‘HACK’ was done, admitting a space leak.
This leak has been noticed several times (prometheus#422, prometheus#550, prometheus#654).
By correctly applying the algorithm from the paper we fix the leak.

I have added unit-tests to show that the behaviour is correct.
I have also added a Benchmark in the benchmark module showing the difference with the old and current
implementation.

According to my benchmarks, is in the new implementation a `get` of a quantile that has ‘seen’ 1 million elements 440 times faster.
Inserting 1 million elements is 3.5 times faster.

While going through the CKMS paper and the Java implementation I have added remarks and snippets
from the paper, to clarify why certain choices are made.
DieBauer added a commit to DieBauer/client_java that referenced this issue Jan 22, 2022
CKMSQuantiles is copied from an implementation of 2012, where it states that a ‘HACK’ was done, admitting a space leak.
This leak has been noticed several times (prometheus#422, prometheus#550, prometheus#654).
By correctly applying the algorithm from the paper we fix the leak.

I have added unit-tests to show that the behaviour is correct.
I have also added a Benchmark in the benchmark module showing the difference with the old and current
implementation.

According to my benchmarks, is in the new implementation a `get` of a quantile that has ‘seen’ 1 million elements 440 times faster.
Inserting 1 million elements is 3.5 times faster.

While going through the CKMS paper and the Java implementation I have added remarks and snippets
from the paper, to clarify why certain choices are made.

Signed-off-by: Jens <jenskat@gmail.com>
fstab pushed a commit that referenced this issue Jan 30, 2022
CKMSQuantiles is copied from an implementation of 2012, where it states that a ‘HACK’ was done, admitting a space leak.
This leak has been noticed several times (#422, #550, #654).
By correctly applying the algorithm from the paper we fix the leak.

I have added unit-tests to show that the behaviour is correct.
I have also added a Benchmark in the benchmark module showing the difference with the old and current
implementation.

According to my benchmarks, is in the new implementation a `get` of a quantile that has ‘seen’ 1 million elements 440 times faster.
Inserting 1 million elements is 3.5 times faster.

While going through the CKMS paper and the Java implementation I have added remarks and snippets
from the paper, to clarify why certain choices are made.

Signed-off-by: Jens <jenskat@gmail.com>
fstab pushed a commit that referenced this issue Jan 30, 2022
CKMSQuantiles is copied from an implementation of 2012, where it states that a ‘HACK’ was done, admitting a space leak.
This leak has been noticed several times (#422, #550, #654).
By correctly applying the algorithm from the paper we fix the leak.

I have added unit-tests to show that the behaviour is correct.
I have also added a Benchmark in the benchmark module showing the difference with the old and current
implementation.

According to my benchmarks, is in the new implementation a `get` of a quantile that has ‘seen’ 1 million elements 440 times faster.
Inserting 1 million elements is 3.5 times faster.

While going through the CKMS paper and the Java implementation I have added remarks and snippets
from the paper, to clarify why certain choices are made.

Signed-off-by: Jens <jenskat@gmail.com>

Fix assertion in HistogramTest

Median of 1..11 = 6

Signed-off-by: Jens <jenskat@gmail.com>

Address PR remarks

Signed-off-by: Jens <jenskat@gmail.com>
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

No branches or pull requests

2 participants