Skip to content
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

Choice of JSON Library | Migrating from RapidJSON #715

Closed
Jan200101 opened this issue Jun 15, 2024 · 9 comments
Closed

Choice of JSON Library | Migrating from RapidJSON #715

Jan200101 opened this issue Jun 15, 2024 · 9 comments

Comments

@Jan200101
Copy link
Contributor

While working on #713 I found out that newer toolchains fail to build RapidJSON due to const-qualified types being kept const Tencent/rapidjson#2277

With RapidJSON not having had a release in almost a decade, maybe we should switch to a better maintained library?

Needs to be discussed.

@Jan200101
Copy link
Contributor Author

Jan200101 commented Jun 15, 2024

Personally I think https://github.com/nlohmann/json would be a good replacement.
It's already considered the go-to C++ JSON library and would allow cmake integration.

@Jan200101
Copy link
Contributor Author

Jan200101 commented Jun 15, 2024

It's been mentioned on discord
RapidJSON is still in development however they stopped tagging commits.
I really hate this because it doesn't give us anchors to work with where we can point at a new release every now and then or where one can say "This fix was included with Version XYZ".
This essentially puts more work on us where we have to keep track of changes ourselves.

We could continue cherry picking fixes or fixing stuff ourselves or move over to a library that respects the ecosystem and didn't just release it for luls like Tencent did.

@Jan200101
Copy link
Contributor Author

Digging through the chat log about previous discussions

[1:53 PM]Gecko: Does the nlohmann support [JSON5](https://json5.org/) ? ^^"

Cause rapidjson allows for comments in JSON which is part of JSON5 spec (but it also doesn't fully support JSON5 cause that would be too easy :dread:)

Basically, if planning to replace rapidjson, the replacement needs to be able to deal with comments (and maybe trailing commas) cause otherwise a bunch of mods will start to fail parsing due to comments in their mod.json...

@Jan200101
Copy link
Contributor Author

It was mentioned that one reason RapidJSON was chosen is performance.
Here are some alternatives which are high performance JSON Libraries which we can use in C++:

Both of which consider themselves fasters than RapidJSON by a large margin.

@GeckoEidechse
Copy link
Member

Depending on how substantial changing JSON library would be, I guess we could treat RapidJSON as a rolling release and use something like dependabot for informing us about new commits.
Of course I understand that this is not optimal so I'd love input from anyone who might have more experience on the matter ^^

@ASpoonPlaysGames
Copy link
Contributor

primedev moved over to nlohmann (or however it is spelt) and that seemed to go fine. Might be relatively trivial to port as well

@Jan200101
Copy link
Contributor Author

I'm also in favor of nlohmann since its saner than RapidJSON and actually makes releases
but when this was discussed on Discord

[1:20 AM]p0358: also nlohmann sucks for performance so
[1:20 AM]p0358: that'd be a downgrade for no particular reason
[1:20 AM]Jan: oh sorry, I didn't know we use RapidJSON every frame
[1:21 AM]p0358: oh sorry, I forgot Northstar already takes long as shit to boot up, might as well propose to make it even longer
[1:21 AM]p0358: and spend yet extra of people's time implementing that

Which is why I am more in favor of yyjson to squash those complaints
(not that RapidJSON was even that fast to begin with, since we never enabled SIMD optimizations)

@GeckoEidechse
Copy link
Member

How much performance impact do we actually have from reading JSON? Like we read enabledmods.json, and the mod.json of however many mods the user has installed. That's it in regards to the launch sequence, right? ^^"

If primedev uses nlohmann as @ASpoonPlaysGames mentioned I'm slightly in favour of using that as well, primarily so that we keep the codebases closer together so that @ASpoonPlaysGames can port over the remaining changes more easily.

To improve launching performance of Northstar we should find out what causes the greatest bottleneck and start there rather than base our choice of JSON library on it when (I assume) reading the JSON files makes up a very minuscule part of the slow launch sequence.

@GeckoEidechse
Copy link
Member

GeckoEidechse commented Jun 30, 2024

Ok, according to @Klemmbaustein nlohmann does not support trailing commas. Given that our JSON parsing is pretty lenient, essentially almost JSON5 we need to keep up that leniency in order to not break too many existing mods.

Similar thing with comments which are used in a bunch of mods and why FlightCore and Viper just assume JSON5 for parsing mod.json. No clue what VTOL does but it also supports comments.

Basically, we need to support

  • // style comments
  • trailing commas
  • (anything else?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

3 participants