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
Conversation
while (written < len) { | ||
b[bOffset + written] = bytearray.get(offset + written); | ||
written++; | ||
// how much can we bulk-read until hitting a multiple of PAGE_SIZE? |
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.
could you put the //comments
behind the actual line - this would be much easier to read and we have 120 chars at least :)
Made args final, reformatted a bit, added PR. |
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 comment
The 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 comment
The 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 comment
The 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 comment
The 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
LGTM |
The initial implementation of bulk-reading a streamInput() over PagedBytesReference was slow (byte-by-byte reading when bulk copying).
Times in µs for bulk-reading a stream over plain vs. paged, averaged over 1000 runs:
This changeset restores performance:
The performance jitters slightly due to the usual Hotspot variances, OS scheduling etc. For all practical purposes the performance is now back to what it was before.