small experiment with cJSON

[garlick]

The cJSON library is small, embeddable, reasonably fast (see JSON benchmarks), and seems widely used. It implements "JSON patch", as described in RFC 6902 which may have applications in the KVS, perhaps as a replacement for the ad-hoc "operation" array in the commit request. I'm curious whether switching to cJSON might not be a useful idea in the KVS internally, particularly if "JSON patch" can be used there. I'm also wondering if this API might be a little nicer than json-c.

In this PR, I embedded the library in flux-core and converted flux-kvs to use it as an experiment. So far, well...not too bad I guess. A couple of interesting bits relative to json-c:

• It allows malloc/free functions to be registered by the user
• it is not refcounted (I'm not sure if that as good or bad at this point!)
• it only implements the "number" type, not int, int64, and double like json-c.
• no name collisions with json-c (unlike jansson) so it doesn't require all or nothing investment

Some problems noted so far

• code is nearly unreadable (e.g. needs to be reformatted IMHO)
• malloc/free pointers are stored in globals which might not work for broker/modules (move to TLS?)
• if you don't register a "fatal" malloc/free, checking for ENOMEM is a bit tricky (e.g. checking for NULL structure members - requires peeking at source)

Not for merging - just a checkpoint.

[coveralls]

Coverage decreased (-0.004%) to 75.341% when pulling 55ec91a on garlick:cjson into dc58fcb on flux-framework:master.

[grondo]

Wow, cJSON kind of feels like the result of late night bet to get a C JSON library under 800 lines. However, it does seem like it is actually used by other projects.

Maybe we could start with cJSON and make something reasonably safe and fast ourselves that implements JSON patch?

[garlick]

Yeah starting with running it through indent!

[codecov-io]

Current coverage is 73.17% (diff: 5.40%)

Merging #833 into master will decrease coverage by 1.86%

@@             master       #833   diff @@
==========================================
  Files           146        148     +2   
  Lines         25415      26097   +682   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          19071      19096    +25   
- Misses         6344       7001   +657   
  Partials          0          0          

Diff Coverage	File Path
0%	new src/common/libcjson/cJSON.c
0%	new src/common/libcjson/cJSON_Utils.c
•••••• 67%	src/cmd/flux-kvs.c

Powered by Codecov. Last update dc58fcb...4882ec1

[coveralls]

Coverage decreased (-0.04%) to 75.303% when pulling f1d06e6 on garlick:cjson into dc58fcb on flux-framework:master.

[coveralls]

Coverage decreased (-0.004%) to 75.341% when pulling 4882ec1 on garlick:cjson into dc58fcb on flux-framework:master.

[garlick]

Looks like code reformatting is in the queue upstream, see DaveGamble/cJSON#24

[grondo]

Just remembered that @trws had checked in rules for clang-format that follow our style guide if indent doesn't give you great results. Try uncommenting the clang-3.7 lines in .clang-format if you have a recent version.

[lipari]

We have RFC 7 that provides guidance on how to make our code look uniformly pretty. I would suggest we add to it whatever tools or options that best do the job.

[garlick]

Thanks guys, I'll probably just wait for the reformatting to come from upstream since it's any day now, and then we'll be able to stay in sync and not subvert automatic rebase of any patches we carry. Assuming this experiment withstands the light of day that is!

[garlick]

For the record, In #301 we discussed replacing json-c at length. For C, it sounded like jansson was the preferred option, but since there are some functions in json-c and jansson that have the same name, we ran into troubles transitioning. As I understand it, libflux had been migrated to jansson in #319, while other parts of flux-core like the kvs module still used json-c. The json-c users would pick up libflux's dependencies when built in-tree via the libtool scripts and get one of the duplicate symbols from the wrong lib.

It was thought that pulling json-c in and building it statically as part of libflux-internal would address this, but for some reason I didn't press ahead and just do that to get us through the transition.

[DaveGamble]

@grondo You are unbelievably close to the truth with your guess as to the origins of cJSON. ;)
If you'd guessed mid-afternoon rather than late-night, you'd have gotten full marks! :D

[grondo]

@DaveGamble, well then kudos are in order! nice job on cJSON, and thanks for the software!

[grondo]

Is this PR dead, Jim?

[garlick]

You mean...

Not quite.

[garlick]

Think I'll withdraw this for now - still on the back burner.

[grondo]

closed by #1988