-
Notifications
You must be signed in to change notification settings - Fork 41
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
JSON in C++ #383
Comments
I guess when we have real use cases. I haven't had one yet. Flux's pack and unpack has been sufficient for me so far. |
#455 clearly indicates that we will absolute need this for better code maintainability. |
One thought I was reminded of earlier this week. I think either of those fit the bill, but it would be really good to pick one that interoperates well with one of the jsonschema implementations for C++ so we don't need to end up with multiple. Might be good to replace the python dep on jsonschema in the long run too if that becomes a performance issue. |
Makes sense. FWIW, for sched for now, python jsonschema isn't on the critical path as I just used it for testing. For actual execution, @grondo and I decided we won't pass an R through jsonschema validation as this is an internal operation and a malformed R will result in an exception raised from the execution system. But in general, it does make sense to have "embedded jsonschema" support as this seems to become trendy with REST API and etc. |
That's a good point. Here are the C++ json-schema validators that I could find (via the json-schema.org implementations list and from googling) and the C++ json parser that they are compatible with:
EDIT: nlohmann's JSON now supports gcc 4.8 |
Probably need draft7 support and C++11. |
I definitely agree that we should limit ourselves to C++11 (and not use C++17, etc). Is there anything, in particular, that we need from draft-5 through draft-7? If not, I think RapidJSON is very appealing as both a C++ library and json-schema validator. It would be just a single new dependency, rather than two, and the JSON parser is extremely fast (faster than jansson IIRC). That being said, nlohmann's JSON for Modern C++ recently added support for gcc 4.8, so that is back to being a viable option for our JSON parser. I updated the table to reflect that. IIRC, this was the C++ JSON parsing library that @trws recommended. Which opens up support for draft-7 via pboettch's jsonschema library. |
The official JGF jsonschema is written for draft4 so I don't have use case for draft 7 yet. If we ever add schema for other formats like rlite we can always just conform to draft4. So if RapidJson looks promising otherwise, this should still be viable. I don't know if jobsprec jsonschema used any other features from draft 4 - 7. |
I like that one mostly for its interface, it’s pretty self-documenting and clean. That said, RAPIDJson is pretty much the industry benchmark for both performance and feature set. The comparison to any of the C json libraries is no contest, from either of these.
https://chadaustin.me/2013/01/json-parser-benchmarking/
It may be possible to write a C json library of similar performance to the C++ ones, but I’ve yet to see any of them even come close for JSON creation (parsing sometimes they get within 2x).
|
Just found two features of draft-5 that would be nice to leverage: v7 also supports comments, which is really helpful considering how complicated these schemas can become. Given this and the previous comments from @dongahn and @trws, I vote that we give nlohmann's JSON for Modern C++ and pboettch's jsonschema libraries a shot. They support draft-v7, are nice interfaces, and work with gcc 4.8. If we hit a deal breaker, we can always fallback to RapidJSON. |
Thank you for the research! Bringing this into flux-sched will substantially simplify our code base and make it less prone to errors. In terms of priorities though, we will need to discuss when would be a good transition point and put a plan around it. |
This is a good point. I'd suggest, as a start, adding in the dependency
and using it for one new feature (possibly schema checking?) for review
rather than making a full transition up front. Making it incremental
but available should keep us from hitting a hitch and dropping forward progress.
"Dong H. Ahn" <notifications@github.com> writes:
… Thank you for the research! Bringing this into flux-sched will substantially simplify our code base and make it less prone to errors. In terms of priorities though, we will need to discuss when would be a good transition point and put a plan around it.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#383 (comment)
|
We decided to stick w/ Jansson. |
Now that more of flux-sched is becoming C++, do we want to keep using the jansson C interface to interact with JSON objects from within C++, or do we want to pull in a C++ JSON library? @trws recommended JSON for modern C++ (which would require a patch to compile under gcc 4.8.5) or RapidJSON.
The text was updated successfully, but these errors were encountered: