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

ResourceLeakDetector sampling changes #7232

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
4 participants
@carl-mastrangelo
Member

carl-mastrangelo commented Sep 20, 2017

Motivation:
Resource Leak Detector (RLD) tries to helpfully indicate where an object was last accessed and report the accesses in the case the object was not cleaned up. It handles lightly used objects well, but drops all but the last few accesses.

Configuring this is tough because there is split between highly shared (and accessed) objects and lightly accessed objects.

Modification:
There are a number of changes here. In relative order of importance:

API / Functionality changes:

  • Max records and max sample records are gone. Only "target" records, the number of records tries to retain is exposed.
  • Records are sampled based on the number of already stored records. The likelihood of recording a new sample is 2^(-n), where n is the number of currently stored elements.
  • Records are stored in a concurrent stack structure rather than a list. This avoids a head and tail. Since the stack is only read once, there is no need to maintain head and tail pointers
  • The properties of this imply that the very first and very last access are always recorded. When deciding to sample, the top element is replaced rather than pushed.
  • Samples that happen between the first and last accesses now have a chance of being recorded. Previously only the final few were kept.
  • Sampling is no longer deterministic. Previously, a deterministic access pattern meant that you could conceivably always miss some access points.
  • Sampling has a linear ramp for low values and and exponentially backs off roughly equal to 2^n. This means that for 1,000,000 accesses, about 20 will actually be kept. I have an elegant proof for this which is too large to fit in this commit message.

Code changes:

  • All locks are gone. Because sampling rarely needs to do a write, there is almost 0 contention. The dropped records counter is slightly contentious, but this could be removed or changed to a LongAdder. This was not done because of memory concerns.
  • Stack trace exclusion is done outside of RLD. Classes can opt to remove some of their methods.
  • Stack trace exclusion is faster, since it uses String.equals, often getting a pointer compare due to interning. Previously it used contains()
  • Leak printing is outputted fairly differently. I tried to preserve as much of the original formatting as possible, but some things didn't make sense to keep.

Result:
More useful leak reporting.

Faster:

Before:
Benchmark                                           (recordTimes)   Mode  Cnt       Score      Error  Units
ResourceLeakDetectorRecordBenchmark.record                      8  thrpt   20  136293.404 ± 7669.454  ops/s
ResourceLeakDetectorRecordBenchmark.record                     16  thrpt   20   72805.720 ± 3710.864  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint              8  thrpt   20  139131.215 ± 4882.751  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint             16  thrpt   20   74146.313 ± 4999.246  ops/s


After:
Benchmark                                           (recordTimes)   Mode  Cnt       Score      Error  Units
ResourceLeakDetectorRecordBenchmark.record                      8  thrpt   20  155281.969 ± 5301.399  ops/s
ResourceLeakDetectorRecordBenchmark.record                     16  thrpt   20   77866.239 ± 3821.054  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint              8  thrpt   20  153360.036 ± 8611.353  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint             16  thrpt   20   78670.804 ± 2399.149  ops/s
} else {
numRecords++;
int numElements = oldHead.pos + 1;
if (dropped = PlatformDependent.threadLocalRandom().nextInt(1 << numElements) >= TARGET_RECORDS) {

This comment has been minimized.

@netkins

netkins Sep 20, 2017

MAJOR Extract the assignment out of this expression. rule

@netkins

netkins Sep 20, 2017

MAJOR Extract the assignment out of this expression. rule

This comment has been minimized.

@normanmaurer

normanmaurer Sep 26, 2017

Member

ignore

@normanmaurer

normanmaurer Sep 26, 2017

Member

ignore

int present = oldHead.pos + 1;
// Guess about 2 kilobytes per stack trace
StringBuilder buf = new StringBuilder(present * 2048).append(NEWLINE);

This comment has been minimized.

@netkins

netkins Sep 20, 2017

MAJOR StringBuffer (or StringBuilder).append is called consecutively without reusing the target variable. rule
MAJOR StringBuffer constructor is initialized with size 16, but has at least 23 characters appended. rule

@netkins

netkins Sep 20, 2017

MAJOR StringBuffer (or StringBuilder).append is called consecutively without reusing the target variable. rule
MAJOR StringBuffer constructor is initialized with size 16, but has at least 23 characters appended. rule

This comment has been minimized.

@normanmaurer

normanmaurer Sep 20, 2017

Member

@netkins is drunk... ignore this

@normanmaurer

normanmaurer Sep 20, 2017

Member

@netkins is drunk... ignore this

Record newHead;
boolean dropped;
do {
if ((prevHead = oldHead = headUpdater.get(this)) == null) {

This comment has been minimized.

@netkins

netkins Sep 20, 2017

MAJOR Extract the assignment out of this expression. rule

@netkins

netkins Sep 20, 2017

MAJOR Extract the assignment out of this expression. rule

This comment has been minimized.

@normanmaurer

normanmaurer Sep 26, 2017

Member

ignore this one

@normanmaurer

normanmaurer Sep 26, 2017

Member

ignore this one

// This will be updated once a new Record will be created and added
private Record tail;
@SuppressWarnings("unchecked") // generics and updaters do not mix.
private static final AtomicReferenceFieldUpdater<DefaultResourceLeak<?>, Record> headUpdater =

This comment has been minimized.

@normanmaurer

normanmaurer Sep 20, 2017

Member

all the joy of java generics

@normanmaurer

normanmaurer Sep 20, 2017

Member

all the joy of java generics

This comment has been minimized.

@carl-mastrangelo

carl-mastrangelo Sep 20, 2017

Member

I tried for at least 30 minutes to get ARFU to match the generics on the inner class. I gave up and went with the cast. The problem is mainly that DefaultResourceLeak.class is static and has the wrong type.

@carl-mastrangelo

carl-mastrangelo Sep 20, 2017

Member

I tried for at least 30 minutes to get ARFU to match the generics on the inner class. I gave up and went with the cast. The problem is mainly that DefaultResourceLeak.class is static and has the wrong type.

This comment has been minimized.

@normanmaurer

normanmaurer Sep 26, 2017

Member

no worries :)

@normanmaurer

normanmaurer Sep 26, 2017

Member

no worries :)

@normanmaurer

A few comments... I would like to cut a release first (tomorrow most likely) and then pull this one in to give it a bit of time for testing etc.

Show outdated Hide outdated common/src/main/java/io/netty/util/ResourceLeakDetector.java Outdated
Show outdated Hide outdated common/src/main/java/io/netty/util/ResourceLeakDetector.java Outdated
Show outdated Hide outdated common/src/main/java/io/netty/util/ResourceLeakDetector.java Outdated
int present = oldHead.pos + 1;
// Guess about 2 kilobytes per stack trace
StringBuilder buf = new StringBuilder(present * 2048).append(NEWLINE);

This comment has been minimized.

@normanmaurer

normanmaurer Sep 20, 2017

Member

@netkins is drunk... ignore this

@normanmaurer

normanmaurer Sep 20, 2017

Member

@netkins is drunk... ignore this

Show outdated Hide outdated common/src/main/java/io/netty/util/ResourceLeakDetector.java Outdated
Show outdated Hide outdated common/src/main/java/io/netty/util/ResourceLeakDetector.java Outdated
Show outdated Hide outdated common/src/main/java/io/netty/util/ResourceLeakDetector.java Outdated
Show outdated Hide outdated common/src/main/java/io/netty/util/ResourceLeakDetector.java Outdated
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Sep 21, 2017

Member

@carl-mastrangelo can you please rebase ?

Member

normanmaurer commented Sep 21, 2017

@carl-mastrangelo can you please rebase ?

@carl-mastrangelo

This comment has been minimized.

Show comment
Hide comment
@carl-mastrangelo
Member

carl-mastrangelo commented Sep 21, 2017

@normanmaurer normanmaurer self-assigned this Sep 26, 2017

@normanmaurer

Just one nit but in general looks awesome to me!

@Scottmitch PTAL as well

Motivation:
Resource Leak Detector (RLD) tries to helpfully indicate where an object was last accessed and report the accesses in the case the object was not cleaned up.  It handles lightly used objects well, but drops all but the last few accesses.

Configuring this is tough because there is split between highly shared (and accessed) objects and lightly accessed objects.

Modification:
There are a number of changes here.  In relative order of importance:

API / Functionality changes:
* Max records and max sample records are gone.  Only "target" records, the number of records tries to retain is exposed.
* Records are sampled based on the number of already stored records.  The likelihood of recording a new sample is `2^(-n)`, where `n` is the number of currently stored elements.
* Records are stored in a concurrent stack structure rather than a list.  This avoids a head and tail.  Since the stack is only read once, there is no need to maintain head and tail pointers
* The properties of this imply that the very first and very last access are always recorded.  When deciding to sample, the top element is replaced rather than pushed.
* Samples that happen between the first and last accesses now have a chance of being recorded.  Previously only the final few were kept.
* Sampling is no longer deterministic.  Previously, a deterministic access pattern meant that you could conceivably always miss some access points.
* Sampling has a linear ramp for low values and and exponentially backs off roughly equal to 2^n.  This means that for 1,000,000 accesses, about 20 will actually be kept.  I have an elegant proof for this which is too large to fit in this commit message.

Code changes:
* All locks are gone.  Because sampling rarely needs to do a write, there is almost 0 contention.  The dropped records counter is slightly contentious, but this could be removed or changed to a LongAdder.  This was not done because of memory concerns.
* Stack trace exclusion is done outside of RLD.  Classes can opt to remove some of their methods.
* Stack trace exclusion is faster, since it uses String.equals, often getting a pointer compare due to interning.  Previously it used contains()
* Leak printing is outputted fairly differently.  I tried to preserve as much of the original formatting as possible, but some things didn't make sense to keep.

Result:
More useful leak reporting.

Faster:
```
Before:
Benchmark                                           (recordTimes)   Mode  Cnt       Score      Error  Units
ResourceLeakDetectorRecordBenchmark.record                      8  thrpt   20  136293.404 ± 7669.454  ops/s
ResourceLeakDetectorRecordBenchmark.record                     16  thrpt   20   72805.720 ± 3710.864  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint              8  thrpt   20  139131.215 ± 4882.751  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint             16  thrpt   20   74146.313 ± 4999.246  ops/s

After:
Benchmark                                           (recordTimes)   Mode  Cnt       Score      Error  Units
ResourceLeakDetectorRecordBenchmark.record                      8  thrpt   20  155281.969 ± 5301.399  ops/s
ResourceLeakDetectorRecordBenchmark.record                     16  thrpt   20   77866.239 ± 3821.054  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint              8  thrpt   20  153360.036 ± 8611.353  ops/s
ResourceLeakDetectorRecordBenchmark.recordWithHint             16  thrpt   20   78670.804 ± 2399.149  ops/s
```
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 7, 2017

Member

@Scottmitch please pull in once satisfied

Member

normanmaurer commented Oct 7, 2017

@Scottmitch please pull in once satisfied

@carl-mastrangelo

This comment has been minimized.

Show comment
Hide comment
@carl-mastrangelo

carl-mastrangelo Oct 17, 2017

Member

@normanmaurer and @Scottmitch

friendly ping. I was hoping this could see some pre release usage. If there are any other concerns, would there be another reviewer to look at this?

Member

carl-mastrangelo commented Oct 17, 2017

@normanmaurer and @Scottmitch

friendly ping. I was hoping this could see some pre release usage. If there are any other concerns, would there be another reviewer to look at this?

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 17, 2017

Member

@carl-mastrangelo feel free to cherry-pick into 4.1 and 4.0. all good imho

Member

normanmaurer commented Oct 17, 2017

@carl-mastrangelo feel free to cherry-pick into 4.1 and 4.0. all good imho

@carl-mastrangelo

This comment has been minimized.

Show comment
Hide comment
@carl-mastrangelo

carl-mastrangelo Oct 19, 2017

Member

Merged as 16b1dbd (4.1) and 74adbcd (4.0)

Member

carl-mastrangelo commented Oct 19, 2017

Merged as 16b1dbd (4.1) and 74adbcd (4.0)

@carl-mastrangelo carl-mastrangelo deleted the carl-mastrangelo:wandermore branch Oct 19, 2017

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Nov 11, 2017

Member

sorry for delay ... I took a look and discussed offline with @carl-mastrangelo

Member

Scottmitch commented Nov 11, 2017

sorry for delay ... I took a look and discussed offline with @carl-mastrangelo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment