Implement optimized version of PinotByteBuffer.#5058
Implement optimized version of PinotByteBuffer.#5058siddharthteotia wants to merge 1 commit intoapache:masterfrom
Conversation
We can directly work on the addresss of the underlying memory chunk bypassing all the chain of function calls and do get/set at the absolute address computed as starting virtual address of underlying memory + offset. Two implementations are provided PinotNativeUnsafeByteBuffer PinotNonNativeUnsafeByteBuffer
|
Codecov Report
@@ Coverage Diff @@
## master #5058 +/- ##
============================================
+ Coverage 58.53% 58.67% +0.13%
Complexity 12 12
============================================
Files 1183 1185 +2
Lines 63213 63515 +302
Branches 9297 9317 +20
============================================
+ Hits 37003 37267 +264
+ Misses 23501 23467 -34
- Partials 2709 2781 +72
Continue to review full report at Codecov.
|
|
This is nice ! Please also test if this compile and running with different java 8+ versions and JDKs. |
| static PinotNativeUnsafeByteBuffer mapFile(File file, boolean readOnly, long offset, int size) | ||
| throws IOException { | ||
| if (readOnly) { | ||
| try (FileChannel fileChannel = new RandomAccessFile(file, "r").getChannel()) { |
There was a problem hiding this comment.
Nit: you can just have the read/write mode assigned based on readOnly flag, and still have the fileChannel.map() and new PinotNativeUnsafeByteBuffer() outside the if check.
| } | ||
|
|
||
| @Override | ||
| public byte getByte(int offset) { |
There was a problem hiding this comment.
Some of these api's seem common in both classes, may be push up to a base class?
| @Measurement(iterations = 5) | ||
| @Fork(1) | ||
| @State(Scope.Benchmark) | ||
| public class BenchmarkPinotByteBuffer { |
There was a problem hiding this comment.
Could you please publish the results of this benchmark and add it to the PR description?
|
|
||
|
|
||
| @ThreadSafe | ||
| public class PinotNativeUnsafeByteBuffer extends PinotDataBuffer { |
There was a problem hiding this comment.
So the caller would need to know and decide whether to use Native vs Non-native? Do you think it would be better to have that switch inside the implementation (so one impl that implements both), so that callers don't need to worry about byte ordering?
|
Re-implemented in #10528 |
6 participants
We can directly work on the address of the underlying memory chunk by-passing all the chain of function calls and do get/set at the absolute address computed as starting virtual address of underlying memory + offset.
Two implementations are provided: