Skip to content
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

Rewrite BytesStreamOutput on top of BigArrays/ByteArray. #5331

Merged
merged 1 commit into from Mar 4, 2014
Merged

Rewrite BytesStreamOutput on top of BigArrays/ByteArray. #5331

merged 1 commit into from Mar 4, 2014

Conversation

hhoffstaette
Copy link

Make BytesStreamOutput more efficient by using paging via BigArrays/ByteArrays. Also changes two existing callers to be more correct/efficient.

Fix for #5159

ensureOpen();

// shrink list of pages
bytes = bigarrays.resize(bytes, BigArrays.PAGE_SIZE_IN_BYTES);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the size was previously less than PAGE_SIZE_IN_BYTES (possible with the contructor that exposes a size), this will actually grow the array (potentially going from a simple heap-allocated byte[] wrapper to a recycling instance)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could be something like:

if (bytes.size() > BigArrays.PAGE_SIZE_IN_BYTES) {
    bytes = bigarrays.resize(bytes, BigArrays.PAGE_SIZE_IN_BYTES);
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default constructor requests at least a single page, so this is simply symmetric. No important client calls this at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I'd rather remove this method, because if they start using it, it will have surprising behavior (I would never expect memory to be allocated in a method called reset)

@jpountz
Copy link
Contributor

jpountz commented Mar 4, 2014

This looks good to me now! +1 to push

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants