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

Allow for ByteBufferInputStream to return underlying ByteBuffer #320

Merged
merged 2 commits into from
May 20, 2016

Conversation

pnarayanan
Copy link
Contributor

@pnarayanan pnarayanan commented May 20, 2016

This helps to avoid a copy if the ByteBufferInputStream consumer needs to get the contents into a ByteBuffer. In particular, this will help avoid a copy when the router needs to convert the ByteBufferInputStream returned by MessageFormat into a ByteBuffer. Rather than read from this stream into an allocated ByteBuffer, the underlying ByteBuffer can be directly used once this patch goes in.

Ming, Gopal. ~10 minutes.

Built and tested on Linux and OS X, and formatted.

This helps to avoid a copy if the ByteBufferInpustStream consumer
needs to get the contents into a ByteBuffer.
stream4.getByteBuffer());
byteBuf.rewind();
ByteBufferInputStream stream5 = new ByteBufferInputStream(byteBuf.duplicate());
ByteBufferInputStream stream6 = new ByteBufferInputStream(stream5, 1024);
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: Can you do byteBuf.remaining() instead of 1024?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was just going with the pattern in the rest of this class. Also, I think the hardcoded value makes sense here as that is the source of truth. Using byteBuf.remaining() might mask issues or catch them later (like if byteBuf was not rewound).

@vgkholla
Copy link
Contributor

LGTM

@pnarayanan
Copy link
Contributor Author

Made another simple required change that might as well go as part of this patch. Please take a look.

@vgkholla
Copy link
Contributor

LGTM

@pnarayanan
Copy link
Contributor Author

@xiahome I am asking @vgkholla to merge this as this is simple enough and getting this merged lets me address your comment in #308 and update it.

Please take a look nonetheless. Will incorporate any comments you might have as part of #308.

@vgkholla
Copy link
Contributor

Merging as per @pnarayanan's comment. @xiahome Please take a look

@vgkholla vgkholla merged commit 058107d into linkedin:master May 20, 2016
@vgkholla
Copy link
Contributor

@pnarayanan Retained your second commit message since it is useful.

@xiahome
Copy link
Contributor

xiahome commented May 20, 2016

LGTM.

@pnarayanan pnarayanan deleted the ByteBufferInputStream branch May 22, 2016 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants