chadaustin / sajson

Lightweight, extremely high-performance JSON parser for C++11
MIT License
568 stars 43 forks source link

No allocation mode for sajson? #26

Closed kccqzy closed 7 years ago

kccqzy commented 7 years ago

Hi, I'm wondering if it is possible to officially support using sajson without any allocation at all. Currently, even if one uses

auto doc = sajson::parse(sajson::single_allocation(buffer, length), sajson::mutable_string_view(length, str));

there will still be one allocation due to the use of refcount in mutable_string_view. In some applications allocations should be avoided as much as possible, but in this case sajson will still call operator new with a size of a size_t.

I have a locally patched version of sajson that removes the use of refcount and changes mutable_string_view to only take unowned strings:

--- a/sajson.h
+++ b/sajson.h
@@ -235,37 +235,13 @@ namespace sajson {
         mutable_string_view()
             : length_(0)
             , data(0)
-            , owns(false)
         {}

         mutable_string_view(size_t length, char* data)
             : length_(length)
             , data(data)
-            , owns(false)
         {}

-        mutable_string_view(const literal& s)
-            : length_(s.length())
-            , owns(true)
-        {
-            data = new char[length_];
-            memcpy(data, s.data(), length_);
-        }
-
-        mutable_string_view(const string& s)
-            : length_(s.length())
-            , owns(true)
-        {
-            data = new char[length_];
-            memcpy(data, s.data(), length_);
-        }
-
-        ~mutable_string_view() {
-            if (uses.count() == 1 && owns) {
-                delete[] data;
-            }
-        }
-
         size_t length() const {
             return length_;
         }
@@ -275,10 +251,8 @@ namespace sajson {
         }

     private:
-        refcount uses;
         size_t length_;
         char* data;
-        bool owns;
     };

     union integer_storage {

I'm thinking perhaps sajson could incorporate something similar officially? Perhaps using an ifdef, or some more templating? If this sounds cool, I can proceed to make a PR.

chadaustin commented 7 years ago

Hi Joe!

I think this is a great idea. I've been pretty busy and haven't had a chance to look more closely into what this would take, but I've never been happy with that refcount allocation. Do you want to try replacing it with move constructors?

We could have an #ifdef for the other stuff, or maybe a template parameter.

I'd support such an effort. :)

Thanks, Chad

chadaustin commented 7 years ago

Similar comments by @iboB in #28. I largely agree, and want to make it possible to use sajson without allocating. On that front, I just committed https://github.com/chadaustin/sajson/commit/39e271cb6ac20081bb7554848529142f54f70836 which only allocates the refcount if a copy of the input buffer must be made.

I'll probably write up a little doc or comment describing how to use sajson without allocating - it may require some sort of bounded AST region.

kccqzy commented 7 years ago

Hey @chadaustin. Sorry for not responding earlier. We are currently using a customized version of sajson with refcount and some other things completely removed. In particular, right now we have a static assert that checks std::is_trivially_destructible<sajson::document>::value. Sorry that I didn't find the time to upstream this properly.

chadaustin commented 7 years ago

Thanks for the heads up! Do whatever you need to. :) One note: you may want to look at cherry-picking a few sajson commits - there were recently a couple important bug fixes. e.g. https://github.com/chadaustin/sajson/commit/0bd8a661421fc3e61262c283543689b0f8a88483

chadaustin commented 7 years ago

I'm going to mark this one as fixed by https://github.com/chadaustin/sajson/commit/39e271cb6ac20081bb7554848529142f54f70836 and continue the conversation in #28. Cheers!