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

unpack: new msgpack_unpacker_next_with_size() function #515

Merged
merged 3 commits into from Sep 11, 2016

Conversation

edsiper
Copy link
Contributor

@edsiper edsiper commented Aug 31, 2016

This new function is an extension of the original msgpack_unpacker_next()
where it now adds third argument to store the number of parsed bytes for
the returned buffer upon a MSGPACK_UNPACK_SUCCESS case.

This is useful for cases where the caller needs to optimize memory usage
in the original buffer,s so upon success retrieval of the object, it can
later deprecate the already 'parsed' bytes.

For more details about the origins of this function please refer to the
following issue on github:

#514

Signed-off-by: Eduardo Silva eduardo@treasure-data.com

This new function is an extension of the original msgpack_unpacker_next()
where it now adds third argument to store the number of parsed bytes for
the returned buffer upon a MSGPACK_UNPACK_SUCCESS case.

This is useful for cases where the caller needs to optimize memory usage
in the original buffer,s so upon success retrieval of the object, it can
later deprecate the already 'parsed' bytes.

For more details about the origins of this function please refer to the
following issue on github:

  msgpack#514

Signed-off-by: Eduardo Silva <eduardo@treasure-data.com>
@redboltz
Copy link
Contributor

redboltz commented Sep 4, 2016

@edsiper , thank you for sending the PR.
I will write comments on the code.

Could you write some tests for this function? The tests should be located on the end of streaming_c.cpp:
https://github.com/msgpack/msgpack-c/blob/master/test/streaming_c.cpp#L122


ret = unpacker_next(mpac, result);
if (ret == MSGPACK_UNPACK_SUCCESS) {
*p_bytes = mpac->parsed;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that p_bytes only be overwritten if ret == MSGPACK_UNPACK_SUCCESS. According to the following comment, even if ret == MSGPACK_UNPACK_CONTINUE, p_bytes only be overwritten should be overwritten as current parsed size.
See #514 (comment)

…ontinue

This patch makes unpacker_next_with_size(...), update p_bytes when
unpacker_next() returns MSGPACK_UNPACK_SUCCESS or MSGPACK_UNPACK_CONTINUE.

Signed-off-by: Eduardo Silva <eduardo@treasure-data.com>
@edsiper
Copy link
Contributor Author

edsiper commented Sep 6, 2016

I wrote a simple test case (to me merged later with the google tests) but I cannot manage to find why it's failing:

#include <stdio.h>
#include <stdlib.h>
#include <msgpack.h>

int main()
{
    int ret;
    size_t bytes;
    size_t parsed = 0;
    msgpack_sbuffer* buffer = msgpack_sbuffer_new();
    msgpack_packer* pk = msgpack_packer_new(buffer, msgpack_sbuffer_write);
    msgpack_unpacked result;
    msgpack_unpacker *unp;

    // 1, 2, 3, "str", ["str_data"], "bin", ["bin_data"], {0.3: 0.4}
    msgpack_pack_int(pk, 1);
    msgpack_pack_int(pk, 2);
    msgpack_pack_int(pk, 3);
    msgpack_pack_str(pk, 3);
    msgpack_pack_str_body(pk, "str", 3);
    msgpack_pack_array(pk, 1);
    msgpack_pack_str(pk, 8);
    msgpack_pack_str_body(pk, "str_data", 8);
    msgpack_pack_bin(pk, 3);
    msgpack_pack_bin_body(pk, "bin", 3);
    msgpack_pack_array(pk, 1);
    msgpack_pack_bin(pk, 8);
    msgpack_pack_bin_body(pk, "bin_data", 8);
    msgpack_pack_map(pk, 1);
    msgpack_pack_float(pk, 0.4f);
    msgpack_pack_double(pk, 0.8);
    msgpack_packer_free(pk);

    unp = msgpack_unpacker_new(32 * 1024);
    msgpack_unpacked_init(&result);

    const char* input = buffer->data;

    while (parsed < buffer->size) {
        memcpy(msgpack_unpacker_buffer(unp), input, 1);
        msgpack_unpacker_buffer_consumed(unp, 1);
        input += 1;

        ret = msgpack_unpacker_next_with_size(unp, &result, &bytes);
        if (ret == MSGPACK_UNPACK_CONTINUE) {
            parsed += bytes;
            continue;
        }

        while (ret == MSGPACK_UNPACK_SUCCESS) {
            parsed += bytes;
            ret = msgpack_unpacker_next_with_size(unp, &result, &bytes);
        }
    }

    printf("parsed=%lu/%lu\n", parsed, buffer->size);
    msgpack_unpacked_destroy(&result);
    msgpack_unpacker_free(unp);
    msgpack_sbuffer_free(buffer);

    return 0;
}

The original buffer size is 48 bytes, but parsed always hits 49. Do you see anything wrong with the above code ?

@redboltz
Copy link
Contributor

redboltz commented Sep 7, 2016

I think that the following part of code is wrong:

        if (ret == MSGPACK_UNPACK_CONTINUE) {
            parsed += bytes;
            continue;
        }

When ret is MSGPACK_UNPACK_CONTINUE, parsed is accumulated. And when ret is MSGPACK_UNPACK_SUCCESS, parsed is accumulated again.

Signed-off-by: Eduardo Silva <eduardo@treasure-data.com>
@edsiper
Copy link
Contributor Author

edsiper commented Sep 7, 2016

you are right!, thanks.

@redboltz redboltz merged commit 87ff7e4 into msgpack:master Sep 11, 2016
@redboltz
Copy link
Contributor

Thank you for updating the PR. merged.

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.

None yet

2 participants