ar90n/msgpack11

Support stream insertion/extraction API

Closed this issue · 5 comments

Hello again!

I've been working to support the C++ streams API on my fork (working branch: https://github.com/alwaysmpe/msgpack11/tree/stream_api) so thought I would open this issue to discuss.

It's a work in progress at the moment, but stream insertion is (I believe) working correctly and I've integrated some tests into the existing code. I will write separate tests but, as I said, it's a work in progress (and I've not used gtest before so getting started quickly by modifying the existing tests). I probably won't be able to finish the stream extraction stuff until next week.

ar90n commented

Hi @alwaysmpe

Thanks for your suggestion.
I think it makes msgpack11 better.

In the your working branch, there are two backends.
The one is conventional string base.
The another is new your ostream base.
I think the conventional backend can be merged into the new one.
Do you think ?

I agree, that makes sense. I didn't want to change too much without making you aware first (which is why i opened the issue). The existing encode API can be easily preserved using a temporary stringstream in the dump() function that uses the stream insertion implementation. I've now amended my previous commit to include this.

I'm now looking at implementing this for extraction from stream.

I agree it makes msgpack11 better, I experimented with several of the C++ implementations of messagepack and this was the most usable and most idiomatically C++ I could find. There are only one or two things missing but it's easier to add things that are missing than change fundamental behaviour! I want to use a stream wrapper for zlib compression to write to file, hence I've been reviewing your code and adding a few missing bits.

I've now got the stream extraction working and all tests are passing. I've also added an additional test to verify that in the case of incomplete data the error is detected. I've not yet set up a pull request, If you have a look and are happy with how it is I'll do that.

The other thing that I was wanting to do was to re-run the benchmark to see if the performance had been negatively impacted, but I couldn't work out how to build/run them, do you have any instructions for that anywhere? It should be a minimal difference, the main cost is likely just increased bounds checking on input (before there was no checking whether the pointer had gone past the end - the stream now abstracts some of this).

ar90n commented

I read codes in your branch.
I think there are no problems and they became more simple by omiting MsgPackParser.
The benchmarking program was designed to use by myself.
So it requires buck which is my best build tool.
If you want to run benchmark by yourself, please run the following commands.

$ buck fetch :msgpack-c-src
$ buck build :benchmark

I put the bechmark result of your branch.
It seems that there are no performance issues.

Library Binary size time[ms] @ Smallest time[ms] @ Small time[ms] @ Medium time[ms] @ Large time[ms] @ Largest
msgpack-c-pack(v2.1.5) 6649 0.62 2.76 48.19 734.17 8581.67
msgpack-c-unpack(v2.1.5) 22034 1.43 6.75 90.24 821.92 10248.81
msgpack11-pack(v0.0.9) 119002 21.23 120.94 989.23 9364.59 117512.33
msgpack11-unpack(v0.0.9) 112704 16.05 105.69 854.58 7540.47 97784.25

Git revision : ff95411

ar90n commented

Closed because #8 was merged.