DL-45: DL should allow ByteBuffer based API and should avoid copying of arrays #21
Conversation
|
the overall change looks good. Let me take a look at the bk change and plan how to merge this. @arvindkandhare |
| import java.util.concurrent.atomic.AtomicInteger; | ||
| import java.util.concurrent.atomic.AtomicReference; | ||
| import java.util.concurrent.locks.ReentrantLock; | ||
|
|
There was a problem hiding this comment.
This looks good, just curious, What is the purpose to move this part down? ：）
There was a problem hiding this comment.
Nothing specific :). I think my editor just sorted all the imports.
|
@arvindkandhare is the twitter/bookkeeper#4 change finalized? |
|
@sijie The BK changes are final and ready to be checked in once you are satisfied with them. This particular pull request still needs a couple of changes. |
|
@arvindkandhare @jiazhai I will merge the bk change and produce a new version this week. |
| this.payload = new byte[length]; | ||
| in.readFully(payload); | ||
| this.payload = ByteBuffer.wrap(new byte[length]); | ||
| //TODO: Fix this |
There was a problem hiding this comment.
what do you want to fix here?
| @@ -290,7 +290,8 @@ public static InputStream fromInputStream(InputStream src, | |||
| src.reset(); | |||
| EnvelopedEntry entry = new EnvelopedEntry(version, statsLogger); | |||
| entry.readFully(new DataInputStream(src)); | |||
| return new ByteArrayInputStream(entry.getDecompressedPayload()); | |||
| return new ByteArrayInputStream(entry.getDecompressedPayload().array()); | |||
There was a problem hiding this comment.
i think im missing something here, but it seems like a bad idea to use direct array access since the payload object could have been initialized with a ByteBuffer from anywhere. is this safe?
| int dataOffset = HEADER_LEN; | ||
| int dataLen = buffer.size() - HEADER_LEN; | ||
|
|
||
| if (COMPRESSION_CODEC_LZ4 != codecCode) { | ||
| ByteBuffer recordSetBuffer = ByteBuffer.wrap(data, 0, buffer.size()); | ||
| ByteBuffer recordSetBuffer = data; |
There was a problem hiding this comment.
i think it would be cleaner if you duplicate() the buffer.. its a little confusing to simply create a new reference.
| this.payload = new byte[length]; | ||
| in.readFully(payload); | ||
| this.payload = ByteBuffer.wrap(new byte[length]); | ||
| //TODO: Fix this |
There was a problem hiding this comment.
will sync with Arvind to get this done.
|
@arvindkandhare @jiazhai I was experimenting using netty-buffer for memory allocation and management. I got pretty good performance results on it. I will try to share more details with numbers in a few days. /cc @leighst |
|
Refer to this link for build results (access rights to CI server needed): |
|
in bookkeeper 4.5, the netty bytebuf is introduced, we might consider using bytebuf there to better integrate with the new bytebuf api. |
|
closed this pull request since DL is using BK 4.5 now. we will pick up the |
This needs the bookkeeper pull request twitter-archive/bookkeeper#4 to be integrated first. Let's review both and then plan a commit plan.