-
Notifications
You must be signed in to change notification settings - Fork 404
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
Implement a pool of reusable JsonGenerator #631
Conversation
Here are some benchmarks against 6.6 (test executed with
|
Implement a pool of reusable JsonGenerator similar to the ReusableByteBufferPool. `CompositeJsonFormatter` now acts as a factory creating JsonFormatter instances bound to an OutputStream. The output stream is backed by a ReusableByteBuffer. CompositeJsonEncoder/CompositeJsonLayout acquire a reusable JsonFormatter from the pool, write the event to it then flush the underlying buffer into a byte array, a string or an another output stream if it implements StreamingEncoder. This approach reuses the JsonGenerator and the associated memory buffers. It also reduces the number of memory copy operations required to move data to destination.
214878c
to
f8d75a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can ReusableByteBufferPool
be deleted? (It looks like this review removes the only references to it, unless I missed something).
src/main/java/net/logstash/logback/composite/CompositeJsonFormatter.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/util/ReusableJsonFormatterPool.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/util/ReusableJsonFormatterPool.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/util/ReusableJsonFormatterPool.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/util/ReusableJsonFormatterPool.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/util/ReusableJsonFormatterPool.java
Outdated
Show resolved
Hide resolved
src/main/java/net/logstash/logback/composite/CompositeJsonFormatter.java
Outdated
Show resolved
Hide resolved
Correct. It is not used anymore. Can be removed unless I find other places where it could be useful ;-) For the background, you may have noticed that ReusableByteBufferPool uses a Deque of SoftReference whereas the ResuableJsonFormatterPool uses a SoftReference holding a Deque (the opposite). I changed the strategy because I noticed that when the GC decides to free the SoftReference it does it for nearly all of them in the same round. It is therefore more efficient to free the Deque instead of individual elements. At least I don't have to re-create a new SoftReference every time elements are added back to the pool... less garbage ;-) |
About disposing JsonGenerator when a pooled JsonFormatter is GC... I had a look and the only consequence of not calling In fact, not closing the JsonGenerator is even better: memory is not returned to Jackson pools and is immediately reclaimed by the GC... which is exactly what we want. ReusableJsonFormatter are disposed by the GC when running low in memory - in this case it is better if the memory allocated by the JsonGenerator is freed as well. |
Do not rely on `#finalize()` anymore to close the underlying JsonGenerator. The JsonGenerator is *not* closed anymore when the ReusableJsonFormatter is disposed. This means that the internal buffers allocated by the generator are not returned to Jackson's internal pool which frees even more memory. Do not recycle a ReusableJsonFormatter after the underlying JsonFormatter threw an exception. The underlying JsonGenerator may be in an unsafe state.
Jackson buffer recycling works by maintaining a pool of buffers per thread. This feature works best when one JsonGenerator is created per thread, typically in J2EE environments. Each JsonFormatter uses its own instance of JsonGenerator and is reused multiple times possibly on different threads. The memory buffers allocated by the JsonGenerator do not belong to a particular thread - hence the recycling feature should be disabled.
@philsttr I'm done with this PR. Everything looks good to me... Any additional comments? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had a few more minor comments. Looks good otherwise!
Implement a pool of reusable JsonGenerator similar to the ReusableByteBufferPool.
CompositeJsonFormatter
now acts as a factory creating JsonFormatter instances bound to an OutputStream. The output stream is backed by a ReusableByteBuffer.CompositeJsonEncoder/CompositeJsonLayout acquire a reusable JsonFormatter from the pool, write the event to it then flush the underlying buffer into a byte array, a string or an another output stream if it implements StreamingEncoder.
This approach reuses the JsonGenerator and the associated memory buffers. It also reduces the number of memory copy operations required to move data to destination.
fixes #630