capnproto / capnproto-java

Cap'n Proto in pure Java
Other
391 stars 86 forks source link

allow to create a MessageBuilder from a MessageReader without copying memory #98

Closed paxel closed 3 years ago

paxel commented 3 years ago

My usecase is, that we have a high performance multi server environment. Java is used to enrich a capnp message that is created by a C/C++ service.

To avoid copying each message in memory it is required to create a MessageBuilder "in place".

paxel commented 3 years ago

I saw that there were already suggestions in previous issues and here also a promise to provide the above fix. I guess it never happened or there were reasons to not do it.

As I understand the proper way to modify an existing message would be to

  1. setRoot on a MessageBuilder // copying the data completely
  2. change the message in the builder
  3. set the root of a second builder with the reader generated from the first builder, to reduce the gaps that might have been created by changing lists and structs // copying the valid data completely
  4. write the builder with Serialize

This pull request at least eliminates the first copy; and if the user can live with unused data in the message also the second.

are there any issues that I overlooked @dwrensha? (Sorry for not providing any tests, my scala skills are not worth mentioning)

paxel commented 3 years ago

It also would allow to forward Messages without copy in a proxy situation. Or is there a way to serialize a MessageReader?

dwrensha commented 3 years ago

One potential issue here is that all of capnproto-java's Builder code does much less validation on data than the Reader code. If you call MessageReader.asBuilder() on untrusted input and try to traverse the message, you could end up in an infinite loop or with index-out-of-bounds errors. So if we add something like this, I would want to make it very clear to users that it's "use at your own risk".

It also would allow to forward Messages without copy in a proxy situation. Or is there a way to serialize a MessageReader?

I don't think we currently have a way to do that, but it should be straightforward to add. It would be similar to this method in capnproto-rust: https://github.com/capnproto/capnproto-rust/blob/0d52dfa85f30125b83adc8af5c52a0fa84f1a7bf/capnp/src/serialize.rs#L290-L291

paxel commented 3 years ago

Sorry for the mess. I probably should close this and redo. But what do you think of the UnsafeOperations solution to make the user aware of him being in dangerous territory?

I also extended Serialize to write/calculate size of MessageReader. (as proposal)

dwrensha commented 3 years ago

I have added support for directly serializing a MessageBuilder in https://github.com/capnproto/capnproto-java/commit/a078df7e4d8789cb04765fbfd79103808fd5eac7.

dwrensha commented 3 years ago

er.. I meant "directly serializing a MessageReader.

dwrensha commented 3 years ago

I added MessageBuilder.unsafeConstructFromMessageReader() in https://github.com/capnproto/capnproto-java/commit/a55b869305f74e87010c17ec77ad2e36c2c6b0d2.

@paxel does that work for you?

paxel commented 3 years ago

Yes. Thank you.

paxel commented 3 years ago

any chance to release 1.8 soonish?

dwrensha commented 3 years ago

done