Replace JavaCPP with hand-written JNI - #102
Conversation
bd0bba8 to
c747337
Compare
TomaszTB
left a comment
TomaszTB
left a comment
There was a problem hiding this comment.
Honestly, this is one of those PRs where I just have to trust that the tests passing means things work well. Left a few comments on things that stood out to me though
| timeout-minutes: 30 | ||
| timeout-minutes: 60 |
There was a problem hiding this comment.
Did the build duration increase? That's a big timeout jump
There was a problem hiding this comment.
I had a hang earlier and this was temporary, reverted
| { | ||
| buffer = ByteBuffer.allocate(1); | ||
| // Direct buffers allow zero-copy JNI via GetDirectBufferAddress. | ||
| buffer = ByteBuffer.allocateDirect(1); |
There was a problem hiding this comment.
This change makes me wonder if we ever used the assumption that the buffer was a Java heap buffer.. I feel like we did. Might be worth double checking, looking for sequential access to the buffer. It wouldn't cause an exception or anything, but it'd be quite slow. Also if we every access the internal byte array, though I hope that would throw an exception (or would it allocate a new byte[]?).
There was a problem hiding this comment.
We possibly did, but we did not use .array() or in the code assume it was heap-backed.
For buffer backed IDLSequences, we do assume heap backed, and that is correct. When messages are deserialized, they get copied from native to heap.
| synchronized (loadLock) | ||
| { | ||
| fastddsjavaTools.retcodeThrowOnError(fastddsjava.fastddsjava_load_xml_profiles_string(xml)); | ||
| fastddsjavaTools.retcodeThrowOnError(us.ihmc.fastddsjava.natives.fastddsjava.loadXmlProfilesString(xml)); |
| private static final int OK = RETCODE_OK(); | ||
| private static final int OK = RETCODE_OK; |
There was a problem hiding this comment.
This isn't an optimization anymore, is it?
2 participants
Summary
fastddsjava)