java: properly implement code_copy_fn #545
Conversation
| return ByteBuffer.allocateDirect(length).put(code, code_offset, length); | ||
| /* Ensure code is returned and both offset/length are positive numbers. */ | ||
| if (code != null && offset >= 0 && length >= 0 && offset <= code.length) { | ||
| int length = Math.min(length, code.length - offset); |
There was a problem hiding this comment.
We can decide whether to do this slicing here or in C.
axic
force-pushed
the
java-codecopy
branch
2 times, most recently
from
5b74558 to
d3d7e59
Compare
axic
force-pushed
the
java-codecopy
branch
3 times, most recently
from
06960d1 to
3ca57ae
Compare
| return ByteBuffer.allocateDirect(length).put(code, code_offset, length); | ||
| /* Ensure code is returned and both offset/length are positive numbers. */ | ||
| if (code != null) { | ||
| // TODO: use wrap here, no need to duplicate this given it is copied again. |
There was a problem hiding this comment.
This TODO says we can use ByteBuffer.wrap, am I reading this right? But for C code to interact with ByteBuffer, you need to use a direct memory allocation. I had panics when I tried to use heap-based bytebuffers.
There was a problem hiding this comment.
I did not see this difference in the JNI docs, only whether it is copied or not. And here we do not need copying given the C counterpart does a copy anyway.
Can you point me to the documentation where this is mentioned?
There was a problem hiding this comment.
I think the code used to get the pointer of the bytebuffer, and that would crash the JVM. So I think you're saying that's all changed now - I'm good with a change there, and I'll make sure to give you a reproduction case if that happens again.
There was a problem hiding this comment.
I think you are right, documentation does seem to claim that allocateDirect will 100% be laid on a way it can be accessed without copying, while other may require copying. It is not clear however who does that copying, but it seems it is done by the JVM so it would mean a slowdown, but not a crash.
I can remove this comment.
|
@chfast which one do you prefer, the first or the second commit? |
axic
force-pushed
the
java-codecopy
branch
4 times, most recently
from
5dc5384 to
85fa5fc
Compare
bindings/java/c/host.c
Outdated
| buffer_data = (uint8_t*)(*jenv)->GetDirectBufferAddress(jenv, jresult); | ||
| (void)buffer_data; | ||
| assert(buffer_data != NULL); | ||
| uint8_t* code = (uint8_t*)(*jenv)->GetDirectBufferAddress(jenv, jresult); |
There was a problem hiding this comment.
This should use GetDirectBuffer from #552.
There was a problem hiding this comment.
You can ignore that, still waiting for an answer on this: #545 (comment)
|
@chfast still needs your review |
|
@chfast okay this is my final proposal. |
| @@ -16,6 +16,13 @@ | |||
| * <p>The set of all callback functions expected by VM instances. | |||
| */ | |||
| final class Host { | |||
| static private ByteBuffer ensureDirectBuffer(ByteBuffer input) { | |||
| // Reallocate if needed. | |||
| if (!input.isDirect()) | |||
There was a problem hiding this comment.
No idea what it does. Is this even tested?
There was a problem hiding this comment.
Not all of them are, but get/set_storage is. This helper enforces that the ByteBuffer is in a format ("direct buffer") which can be queried from JNI (e.g. the GetDirectBuffer helper in C won't assert). The tests always use "direct buffer" though so we never seen an assert.
No description provided.