dropbox / json11

A tiny JSON library for C++11.
MIT License
2.55k stars 613 forks source link

allow to parse const char * without mem allocation #60

Open Dushistov opened 8 years ago

Dushistov commented 8 years ago

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 commented 8 years ago

Automated message from Dropbox CLA bot

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

Dushistov commented 8 years ago

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

j4cbo commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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));
}