Add functions to serialize to and deserialize from memory buffers#18
Conversation
denialhaag
force-pushed
the
improve-cmake
branch
from
38f05fe to
fb3be2d
Compare
denialhaag
changed the title
|
Some context for @dime10: Our round-trip tests in MQT Core didn't serialize and deserialize the converted |
| writeMessage(module, message); | ||
| capnp::writeMessage(output, message); | ||
| auto bytes = serialize(module); | ||
| output.write(reinterpret_cast<const char*>(bytes.data()), bytes.size()); |
There was a problem hiding this comment.
The split is definitely good, but I think we are introducing 2 copies here to write to file? The messageToFlatArray and then again to SmallVector, before then going to file.
Some references that might be relevant:
https://groups.google.com/g/capnproto/c/NBayPDP0_84/m/1meBteW2CAAJ
https://capnproto.org/cxx.html#messages-and-io
There was a problem hiding this comment.
@burgholzer I might be interesting if you could take a quick look at these links and contrast to your approach. If I understand your current code correctly, we are making one copy to serialize, and no copies to deserialize? (not including file i/o)
I suppose in theory one could use the memory buffer of the message builder directly for a zero copy approach, or could that pose issues?
There was a problem hiding this comment.
I did another iteration on the serialization and deserialization code.
I think both serialization and deserialization should now be zero copy.
Maybe you could have another look 👀
|
Sorry @dime10 for wasting some of your time. I was in the middle of working on the PR and did not notice your review in the middle of it. |
No worries, I think we just happened to have worked on it at the same time 😅 it's all good |
dime10
left a comment
dime10
left a comment
There was a problem hiding this comment.
Thanks a lot @burgholzer for those improvements!
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
Signed-off-by: Lukas Burgholzer <burgholzer@me.com>
|
Alright, this is now finally also working on Windows in munich-quantum-toolkit/core#1676 At some point, we may want to consider also expanding the CI matrix here to catch such failures earlier. |
| @@ -1,5 +1,8 @@ | ||
|
|
||
| +# Disable vendored Cap'n Proto tests when consumed through FetchContent. | ||
| +set(BUILD_TESTING OFF) |
There was a problem hiding this comment.
If it's just a matter of the flag, can we not set it from our side?
There was a problem hiding this comment.
Unfortunately not. The project itself calls include(CTest), which unconditionally sets that variable again and overrides anything that we set here.
I have probably tried 5 different ways to exclude the tests; this is the one that worked reliably.
Mid- to long-term, one could contribute an upstream fix for this that scopes the testing to the project.
The sensible default is typically to enable the tests by default if running as the top-level CMake project (using PROJECT_IS_TOP_LEVEL) and to provide an additional project-specific option in CMake that defaults to OFF for downstream consumers.
I currently do not have the capacity to drive that through, but maybe @denialhaag has once he is back from vacation.
3 participants
Fixes #6
Fixes #8