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

Update WireHelpers.java to improve data-blob copying by bulk put #143

Merged
merged 2 commits into from
Oct 22, 2024

Conversation

stepeos
Copy link
Contributor

@stepeos stepeos commented Oct 20, 2024

Hello maintainers!
For context: I am using capnproto-java for image processing pipelines in kotlin on android. For this matter, there is a lot of ByteArrays for data buffers of image blobs.

Now while profiling I saw that the data copying has some nasty CPU-usage:
image
Almost a third of the copy operation is checkIndex().

And in the source code I saw this TODO.

I have seen that bulk ByteBuffer.put(byte[], int, int) and ByteBuffer.put(ByteBuffer serc) is available since Java 8. Is there a reason not to for minimal Java version?

I see the same issue in the code with getWritableDataPointer without taking a closer look.

Cheers
Steve

@stepeos stepeos changed the title Update WireHelpers.java Update WireHelpers.java to improve data-blob copying by bulk put Oct 20, 2024
@stepeos
Copy link
Contributor Author

stepeos commented Oct 21, 2024

i was thinking of using put(byte[] int, int length) instead, since the buffers' positions will be changed.
However, both methods act on the position so there is no thread safe way to do this, or am I wrong?
I don't have the time to dig deeper here, and for my usecase; my change already fixes my issue, but I'll leave this as a thought.

@dwrensha dwrensha merged commit 08009f3 into capnproto:master Oct 22, 2024
1 check passed
@dwrensha
Copy link
Member

Looks good to me. Thanks!

@stepeos
Copy link
Contributor Author

stepeos commented Oct 22, 2024

@dwrensha what about the other functions with the same code?
I pointed out getWritableDataPointer for instance. Want me to PR or are you doing that?

Also, is it thread safe? Because I am changing the ByteBuffer position?

@dwrensha
Copy link
Member

I opened #144 for the other TODOs.

@stepeos
Copy link
Contributor Author

stepeos commented Oct 22, 2024

perf, thanks!

@dwrensha
Copy link
Member

Hm... setTextPointer uses duplicate():

ByteBuffer slice = value.buffer.duplicate();
slice.position(value.offset);
slice.limit(value.offset + value.size);

I suppose that is more thread-safe?

@dwrensha
Copy link
Member

I just pushed 28ab5ce

@dwrensha
Copy link
Member

I added a test in cfea947.

The size member of Data.Reader sometimes is smaller than the limit of the buffer member. This is another reason to use ByteBuffer.duplicate().

@stepeos
Copy link
Contributor Author

stepeos commented Oct 22, 2024

Ah thanks 👍, I tried avoiding the duplicate copying but it makes sense I see!

@dwrensha
Copy link
Member

For the record, duplicate() does not copy the buffer's content. It just creates a new wrapper for it: https://docs.oracle.com/javase/8/docs/api/java/nio/ByteBuffer.html#duplicate--

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.

2 participants