-
Notifications
You must be signed in to change notification settings - Fork 24.2k
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
Restore streamInput() performance over PagedBytesReference. #5589
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -381,12 +381,14 @@ private int countRequiredBuffers(int initialCount, int numBytes) { | |
private static class PagedBytesReferenceStreamInput extends StreamInput { | ||
|
||
private final ByteArray bytearray; | ||
private final BytesRef ref; | ||
private final int offset; | ||
private final int length; | ||
private int pos; | ||
|
||
public PagedBytesReferenceStreamInput(ByteArray bytearray, int offset, int length) { | ||
this.bytearray = bytearray; | ||
this.ref = new BytesRef(); | ||
this.offset = offset; | ||
this.length = length; | ||
this.pos = 0; | ||
|
@@ -420,7 +422,7 @@ public int read() throws IOException { | |
} | ||
|
||
@Override | ||
public int read(byte[] b, int bOffset, int len) throws IOException { | ||
public int read(final byte[] b, final int bOffset, final int len) throws IOException { | ||
if (len == 0) { | ||
return 0; | ||
} | ||
|
@@ -430,17 +432,25 @@ public int read(byte[] b, int bOffset, int len) throws IOException { | |
} | ||
|
||
// we need to stop at the end | ||
len = Math.min(length, len); | ||
int todo = Math.min(len, length); | ||
|
||
// ByteArray.get(BytesRef) does not work since it flips the | ||
// ref's byte[] pointer, so for now we copy byte-by-byte | ||
// current offset into the underlying ByteArray | ||
long bytearrayOffset = offset + pos; | ||
|
||
// bytes already copied | ||
int written = 0; | ||
while (written < len) { | ||
b[bOffset + written] = bytearray.get(offset + written); | ||
written++; | ||
|
||
while (written < todo) { | ||
long pagefragment = PAGE_SIZE - (bytearrayOffset % PAGE_SIZE); // how much can we read until hitting N*PAGE_SIZE? | ||
int bulksize = (int)Math.min(pagefragment, todo - written); // we cannot copy more than a page fragment | ||
boolean copied = bytearray.get(bytearrayOffset, bulksize, ref); // get the fragment | ||
assert (copied == false); // we should never ever get back a materialized byte[] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this still confuses me why do we return that boolean if it is always expected to be false? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I your reply got busted.... lemme reread There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The assert was just for testing. If you find it less confusing I can remove the return value & the assert . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am just wondering if we can make sure we never get a class taht does that but I guess the assert is ok |
||
System.arraycopy(ref.bytes, ref.offset, b, bOffset + written, bulksize); // copy fragment contents | ||
written += bulksize; // count how much we copied | ||
bytearrayOffset += bulksize; // advance ByteArray index | ||
} | ||
|
||
pos += written; | ||
pos += written; // finally advance our stream position | ||
return written; | ||
} | ||
|
||
|
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 as a sidenote I think the arguments should be final to make sure we don't reassign them :)