Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

allow to parse const char * without mem allocation #60

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Dushistov
Copy link
Contributor

At now, if you have json data in std::vector<char>, QByteArray (from Qt) or const char *, then to parse json with json11 you need to do unnecessary memory allocation to convert (pointer, size) pair to
std::string. This patch replace std::string interface for parsing with simple structure string_view = pair of pointer and size.

native json benchmark (https://github.com/miloyip/nativejson-benchmark) show:

without this patch Parse Time (ms) - 51
with this patch Parse Time (ms) - 8,837

benchmark uses parse(const char * in,...) interface.

string_view (pair of pointer and size) implement part of interface of std::basic_string_view(http://en.cppreference.com/w/cpp/string/basic_string_view).

@smarx
Copy link

smarx commented May 9, 2016

Automated message from Dropbox CLA bot

@Dushistov, it looks like you've already signed the Dropbox CLA. Thanks!

@Dushistov
Copy link
Contributor Author

I have already signed the copyright assignment, when creating other pull requests.

@j4cbo
Copy link
Contributor

j4cbo commented May 9, 2016

I'm not really comfortable implementing a part of experimental::string_view on its own, since that'll cause compatibility issues with other parts of a project that might have their own. I think it'd be better to just pass a const char * and length around internally.

@Dushistov
Copy link
Contributor Author

Dushistov commented May 9, 2016

since that'll cause compatibility issues with other parts of a project that might have their own

it is inside json11 namespace and all methods of it are inline, so it not cause compile time errors,
and not cause unnecessary code bloat in resulted executable.

I think it'd be better to just pass a const char * and length around internally.

I thought about it, but if you use std::pair<const char *, size_t> = struct Foo { const char *data; size_t len;} or just pass around poinster and length, you in fact implement string_view, at least compare, substr, operator std::string, constructor (to accept std::string,const char *,const char *, size_t) and so on.

So you will have string_view with different name, and guy who look at code after you will say "Hey, they just re-implemented string_view, why?`, so why hide truth and not explicity implement string_view and add note about it?

One more possible solution may be

namespace json11 { namespace Private { class string_view; }

template<class string_view_imp = Private::string_view>
class Json final {
};
}

So you can replace string_view implementation to your own internal, or just
reuse std::string_view if your compiler + std libarary support it,
and that's all with just one typedef/using.

What do you think?

@artwyman
Copy link
Contributor

You might be able to minimize the internal impact without forcing the class itself to be a template, if you have a template method which takes something string-view-like. E.g. implement your own internal string-view-like thing (simpler than the real string view since you only need a subset of functionality), then have a method like this:

template <typename StringView>
static Json parse(const StringView& in,
                    std::string & err,                             
                    JsonParse strategy = JsonParse::STANDARD) {
    return parse(internal_string_rep(in.data(), in.length())); // Calls private method with private type
}

That should work for any input type which supports data() and length(), including std::string and std::experimental::string_view. For full flexibility I'd suggest also providing a ptr+length form of the method:

static Json parse(const char * in_ptr, size_t in_len,
                    std::string & err,                             
                    JsonParse strategy = JsonParse::STANDARD) {
    return parse(internal_string_rep(in_ptr, in_len));
}

Copy link
Contributor

@artwyman artwyman left a comment

Choose a reason for hiding this comment

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

Invoking the new code-review process for this. Comment/requests are in the previous discussion above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants