-
Notifications
You must be signed in to change notification settings - Fork 35
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
Use BufferPool
for cleartext as well as ciphertext chunks
#125
Conversation
BufferPool
for cleartext as well as ciphertext chunks
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.
Amazing work you did here 👍
@overheadhunter Did you considered the Chunk
class implementing Autoclosable
? In the close method, it could automatically call bufferpool.recycle()
.
- Chunk implementing Autoclosable
src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java
Outdated
Show resolved
Hide resolved
The problem is that Chunk is passed around between classes a lot, i.e. try-with-resource can't be used. It is even cached, so analysis tools have a hard time tracking the lifecycle and will report false positive leaks. |
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 one naming thing
ChunkData chunkData = ChunkData.emptyWithSize(cleartextChunkSize); | ||
chunkData.copyData().from(src); | ||
chunkCache.set(chunkIndex, chunkData); | ||
ByteBuffer cleartextChunk = bufferPool.getCleartextBuffer(); |
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.
should be named cleartextBuffer
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.
But it is a chunk. I prefer to name variables according to semantics rather than to types.
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.
Correct, but from a technical perspective a chunk is an object derived from the Chunk
class wrapping a buffer. Here those two definitions intersect, and to avoid confusion I would tend to the technical description. I mean, we are wrapping three lines later a "chunk" with "Chunk". And at creation, the "chunk" is not a "chunk" but just a buffer (the method even says it). Additionally, the naming scheme is also used in other places.
As a compromise, how about renaming it to cleartextChunkData
?
Kudos, SonarCloud Quality Gate passed! |
Previously, each chunk of data allocated a new 32kB ByteBuffer, which was in most cases very short-lived. This lead to both, massive allocations as well as garbage collection activity.
To reduce both memory as well as CPU pressure, this introduces a filesystem-scoped pooling mechanism. ByteBuffers should be returned to the pool but it will do no harm not to do so.