channels/Common: only allocate temp buffers for read and write if nec… #49
Conversation
…essary jnr-ffi seems to be ok with array backed or direct buffers, so just pass those right on through. Signed-off-by: Samuel Just <[email protected]>
|
Should fix #48 |
|
Seems it failed CI due to: 3.04s$ mvn install -DskipTests=true -Dmaven.javadoc.skip=true -B -V which I think is unrelated? |
|
@headius Does this look right? |
| @@ -44,13 +44,19 @@ int getFD() { | |||
|
|
|||
| int read(ByteBuffer dst) throws IOException { | |||
|
|
|||
| ByteBuffer buffer = ByteBuffer.allocate(dst.remaining()); | |||
| int n; | |||
| if (!dst.hasArray() && !dst.isDirect()) { | |||
There was a problem hiding this comment.
Why do this additional check at all? Why not call Native.read directly with dst. I can't see why the new allocation is required even for non-direct mode. Native.read appears to be agnostic to the type of byte buffer, and if there is something to be done, then the JNI call-site is the best place for those concerns.
| @@ -92,13 +98,18 @@ long read(ByteBuffer[] dsts, int offset, int length) | |||
|
|
|||
| int write(ByteBuffer src) throws IOException { | |||
|
|
|||
| ByteBuffer buffer = ByteBuffer.allocate(src.remaining()); | |||
| int n; | |||
| if (!src.hasArray() && !src.isDirect()) { | |||
|
@athanatos Nothing appears weird about Travis - perhaps retry it? |
|
@headius Are you the best committer to review/merge once the issues are sorted? |
|
|
||
| buffer.put(src); | ||
| buffer.put(src); |
There was a problem hiding this comment.
this is incorrect as per #50 as the src position is updated to limit, which it should be updated by n (bytes written)
|
@athanatos Did you get a chance to address @gregw review comments? I can spin a release with this update any time, if we get those issues tidied up. |
|
I'm going to close this for lack of activity. If you can rebase on master and address review comments, we can still get this in! |
…essary
jnr-ffi seems to be ok with array backed or direct buffers, so just pass
those right on through.
Signed-off-by: Samuel Just [email protected]