[json][review] JSON review
We vote to reject the JSON library from Boost and our review of the library and reason for rejection are below: DOCUMENTATION The documentation is very good. The usage and examples and reference are easy to understand and contain all the information we are looking for. IMPLEMENTATION The organization of source code is very messy: 1. Some files do not include all headers at the very start of the file but have some include statements at the end. For example the json/object.hpp file has 10 include statements at the start of the file before the opening namespace and then 1 include statement at the end of the file after the closing namespace. This is very confusing and not a good practice. 2. Splitting the files into HPP and IPP is also not good because it spreads the class into more files than necessary and causes compilation to be slower. Using the library in a project has a complication: 3. The library is complicated to use in a project because you cannot simply include headers in any file you want. You have to include a src.hpp file but this file must be included only once. Normal header libraries in Boost just let you include the headers without this strange requirement. The library adds a lot of size to your executable: 4. Unlike other JSON libraries which are smaller, this adds several KB to the exe size. This is possibly because of use of templates or because it contains very big lookup tables. The performance is not as fast for all use cases: 5. Compared to our in house JSON library, this library was slightly slower. We have very simple but very large JSON data. We use objects and arrays, but only integer numbers with no fractional part and very large ASCII strings, and no booleans and no nulls. In one of our standalone conversion tools we tried using this library and found it had a 4% longer runtime on a small 170 MB file. The parser allocates too much memory: 6. I think this is because the design is not optimized for the case when all the JSON content is already stored in memory. In our parser we point to the strings in the original buffer. The library duplicate code that is in the C++ compiler libraries: 7. In json/config.hpp there are implementations of move and remove_cv from the C++ standard. This introduces more duplicate of this functionality into programs. Besides these points the implementation is well written. DESIGN The allocator design is not standard: 1. The library has a different allocation system that is not the C++ standard system for allocators. We did not understand why storage_ptr was needed at first. It would be better for the library to just template on allocator just like the vector class in the C++ standard. The allocator system also uses virtual calls. The C++ standard vector class also does not have this problem when it uses the standard allocator system. Other than these points the design is ok. OVERALL We would not use this library because it does not offer anything special for JSON that we need. It also uses some designs that are not standard practice. We use JSON heavily in our historical market quote service and most of our tools deal with JSON files and we use many Boost libraries so we are hoping for a JSON library in Boost that we could use instead of having our own.
On Mon, Sep 21, 2020 at 11:04 AM Harshada Wighe via Boost
We vote to reject the JSON library from Boost and our review of the library and reason for rejection are below:
Thank you for spending the time to review this library.
1. Some files do not include all headers at the very start of the file but have some include statements at the end.
Yes this is explained in the comment: https://github.com/CPPAlliance/json/blob/6ddddfb16f9b54407d153abdda11a29f746...
2. Splitting the files into HPP and IPP is also not good because it spreads the class into more files than necessary and causes compilation to be slower.
Did you measure? .ipp files are compiled into the library. This arrangement actually causes compilation to be faster, as the library's function definitions are only compiled once and not included in every one of the user's translation units.
3. The library is complicated to use in a project because you cannot simply include headers in any file you want. You have to include a src.hpp file but this file must be included only once.
No that's not correct. Boost.JSON can be used just like every other Boost library. You include the public headers you want, and then link with the compiled library. This is explained in the documentation: https://master.json.cpp.al/json/overview.html#json.overview.requirements.usi... "src.hpp" provides an alternative to linking with a compiled library, it is another way to use Boost.JSON.
5. Compared to our in house JSON library, this library was slightly slower.
Sample code please.
The parser allocates too much memory:
6. I think this is because the design is not optimized for the case when all the JSON content is already stored in memory. In our parser we point to the strings in the original buffer.
Instances of json::value produced by the parser are first-class, Regular data types; they are copyable, movable, assignable, etc... "in-situ parsing" (what you refer to as pointing to strings in the original buffer) is a specialized use-case which does not produce Regular data types. In other words it is not a focus of Boost.JSON.
7. In json/config.hpp there are implementations of move and remove_cv from the C++ standard. This introduces more duplicate of this functionality into programs.
This is removed in the develop branch, it was an early experiment to reduce the amount of header material brought in from the standard library. Boost.JSON design choices emphasize speed of compilation.
The allocator design is not standard:
std::pmr::memory_resource is standard as of C++17. For earlier versions of C++, boost::container::pmr::memory_resource is used.
We did not understand why storage_ptr was needed at first.
Explained in the documentation: https://master.json.cpp.al/json/usage/allocators/background.html
It would be better for the library to just template on allocator just like the vector class in the C++ standard.
I disagree. Templates are completely unnecessary for memory allocation, as this library demonstrates. Making json::value be a template would put all of the function definitions into the public headers, and increase the size of the executable when different allocators are instantiated in the same program. It would also hinder interoperability: every library that wants to use JSON types in its public interfaces would then be required to use function templates, or to pick an arbitrary Allocator type. Neither of these are desirable outcomes.
The allocator system also uses virtual calls. The C++ standard vector class also does not have this problem when it uses the standard allocator system.
std::pmr::vector, which uses std::pmr::polymorphic_allocator is standard as of C++17. For earlier versions of C++, boost::container::pmr::vector is available which uses boost::container::pmr::polymorphic_allocator. You have not stated why virtual calls are a problem. Did you measure? Regards
On Mon, Sep 21, 2020 at 2:04 PM Harshada Wighe via Boost < boost@lists.boost.org> wrote:
5. Compared to our in house JSON library, this library was slightly slower. We have very simple but very large JSON data. We use objects and arrays, but only integer numbers with no fractional part and very large ASCII strings, and no booleans and no nulls. In one of our standalone conversion tools we tried using this library and found it had a 4% longer runtime on a small 170 MB file.
The parser allocates too much memory:
6. I think this is because the design is not optimized for the case when all the JSON content is already stored in memory. In our parser we point to the strings in the original buffer.
What you describe here is in-situ parsing. This is not what Boost.JSON aims to provide, nor is it feasible for an incremental parser (in the vast majority of use-cases). As an aside, I'm quite surprised that it performed so well against an in-situ parser. If your in-house library does not validate UTF-8 byte sequences, consider turning it off for Boost.JSON in your benchmark. It could result in a considerable increase in performance.
participants (3)
-
Harshada Wighe -
Krystian Stasiowski -
Vinnie Falco