hse-project / hse

HSE: Heterogeneous-memory storage engine
https://hse-project.github.io
673 stars 65 forks source link

Replace cJSON with another JSON library #580

Open tristan957 opened 1 year ago

tristan957 commented 1 year ago

Description

Problems with cJSON:

  1. Undermaintained
  2. "Lots" of allocations (how big of an issue is this? not sure)
  3. Will leak memory depending on which cJSON APIs you use

Alternatives:

  1. yyjson
  2. jansson
  3. Wrap a C++ JSON library to create a C library

cc @alexttx

tristan957 commented 1 year ago

A C++ library to wrap could be https://github.com/nlohmann/json. It is extremely popular

beckerg commented 1 year ago

"We used all the operator magic of modern C++ to achieve the same feeling in your code."

That's kinda frightening! Why not yyjson?

tristan957 commented 1 year ago

We can use it. I am just giving a wide array of options.

alexttx commented 1 year ago
library stars watchers forks
cJSON 8.3k 271 2.8k
yyjson 2.3k 54 220
jansson 2.7k 125 772
nlohmann/json 33.2k 756 5.7k
tristan957 commented 1 year ago

One thing Greg had suggested was creating like a libhse-json that was just a wrapper around whatever library we choose, so that if we ever had to swap it out, it would be pretty straightforward to do in the future. Technically that work could happen right now wrapping cJSON.

But I am not sure how much gain there is in that approach.

tristan957 commented 1 year ago

What criteria should we use to evaluate this?

tristan957 commented 1 year ago

Last release of jansson: 9/9/2021 Last release of yyjson: 12/12/2022 Last release of nlohmann/json: 8/12/2022

beckerg commented 1 year ago

jansson looks decent, but i didn't really dig in. the extra maturity could be a good thing...

tristan957 commented 1 year ago

I edited Alex's comment with the table to include the cJSON stats. Technically cJSON is "mature", but we have a lot of problems with it that can't be fixed because it is unmaintained.

The one thing nlohmann/json has going for it is that it has at least one corporate sponsor.

beckerg commented 1 year ago

cJSON may be mature, but the using doubles for integers stupidity is broken by design. So we need one that has actual support for 64-bit integers and is reasonably easy to use. yyjson looks pretty C-ish, jansson looks like you need to learn a bunch of magic printf-like escapes. But I haven't dug in much, would need to see some examples in-situ...

alexttx commented 1 year ago

One advantage of a wrapper is the ability to test the different libraries for performance, memory usage, etc. If the underlying APIs are too different, the wrapper may be more trouble than its worth. It might also be forced into adding its own overhead.

tristan957 commented 1 year ago

yyjson is different enough from the others that it would probably be difficult to write a good wrapper that could abstract everything well since yyjson has a notion of immutability.

alexttx commented 1 year ago

So the only way to instantiate a JSON object in yyjson to parse a string? Does that work for HSE?

tristan957 commented 1 year ago

No. When you parse a file you get a different struct type in yyjson than you do if you are building a json object by hand.