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

New ultra-fast JSON Parser and Reader #3285

Open
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

znvjder
Copy link
Contributor

@znvjder znvjder commented Jan 7, 2024

This pull request introduces a replacement of the "old" and collision-prone json-c library with rapidjson. Rapidjson is a memory-friendly library (each value occupies less than 16 bytes on 32/64-bit devices), it is fast and independent of external libraries, additionally accelerated by SSE2/SSE4.2 technology. All issues associated with the old JSON library have been resolved.

The old "json-c" library has been removed and replaced with the new one. The toJSON and fromJSON functions now include a parser and a reader, modified to maintain backward compatibility, as discussed on the "dev" channel on Discord.

New functions:

json.encode(var variable, bool pretty)
json.decode(string str)

These functions have been added to the global json table and are accessible from there.

Example:

local result = json.decode('{"name": "hello", "array": [1,2,3], "double": 3.1320130, "asint": 12000}')
iprint("result decode", result) 
iprint("result encode", json.encode(result)) 

Performance Testing...
The data writing speed (using rapidjson) is currently, based on tests, approximately 100% faster than the current implementation. Similarly, for the parser.

In general, these results may vary and depend on the hardware used for testing. However, better hardware typically yields better results.

This pull request has been compiled and tested on the Windows 10 x64 and Linux Debian 11 (ARM64) platforms.

Issues addressed and potentially resolved in this pull request:
#1946 #1931 #1073 #1074

Edited by replacing the simdjson parser with rapidjson.

@tederis tederis added the enhancement New feature or request label Jan 7, 2024
@Pirulax
Copy link
Contributor

Pirulax commented Jan 8, 2024

Tough I did suggest simdjson I was under the impression that it has writer functionality too.
I think just having rapidjson would be enough, having 2 JSON libs (or even 3, not sure if we're dropping json-c) is too much.

@znvjder
Copy link
Contributor Author

znvjder commented Jan 9, 2024

Tough I did suggest simdjson I was under the impression that it has writer functionality too.

I think just having rapidjson would be enough, having 2 JSON libs (or even 3, not sure if we're dropping json-c) is too much.

Okay, I can rewrite (parser Part) it using RapidJSON, no problem.

@Disinterpreter
Copy link
Member

Tough I did suggest simdjson I was under the impression that it has writer functionality too.
I think just having rapidjson would be enough, having 2 JSON libs (or even 3, not sure if we're dropping json-c) is too much.

Okay, I can rewrite (parser Part) it using RapidJSON, no problem.

May I ask? Have the point to do new functions? Or it's better to replace exists with breaking compatibility on new version? (Exists functions has bug from their implementation)

@znvjder
Copy link
Contributor Author

znvjder commented Jan 13, 2024

Tough I did suggest simdjson I was under the impression that it has writer functionality too.
I think just having rapidjson would be enough, having 2 JSON libs (or even 3, not sure if we're dropping json-c) is too much.

Okay, I can rewrite (parser Part) it using RapidJSON, no problem.

May I ask? Have the point to do new functions? Or it's better to replace exists with breaking compatibility on new version? (Exists functions has bug from their implementation)

As discussed earlier, we have decided to create an additional json.decode and json.encode function to maintain backward compatibility for the current ones. For example: toJSON function returns JSON as an array, while we expected an object.

The other bugs, aside from returning as an array, have been fixed using the new parser.

Link to the discussion:
https://discord.com/channels/801330706252038164/801411628600000522/1192934310622269581

@znvjder znvjder marked this pull request as ready for review January 13, 2024 21:04
@znvjder
Copy link
Contributor Author

znvjder commented Jan 13, 2024

I'm closing the draft, so feel free to review. :)

PR was compiled and tested on Windows 10 x64 and Linux Debian 11 (ARM64).

@Pirulax
Copy link
Contributor

Pirulax commented Jan 14, 2024

I'd advocate moving rapidjson to a submodule
Not sure what other's think

@TheNormalnij
Copy link
Contributor

I'd advocate moving rapidjson to a submodule

It looks like an universal way to make dependency. I like this idea
Pros:

  • Updates are much easier.
  • You can see changes in the submodule from your git client.
  • It's much easier to updated submodule
  • Less premake5 scripts. So we can switch to cmake in the future.
  • Download speed is better for updates
    Cons:
  • Slower when repo is used first time.
  • 3d-party repo can be vandalized/deleted. This point is valid for premake modules too.
  • CI/CD should be prepared for this.
  • Submodules can be used only for git submodules
  • Can't be used for prebuilded libraries (CEF)

rapidjson is not under development ( last commit was 2 years ago ). I think we can keep sources in our repo.
@znvjder you have sources in the repo and you download them at the same time. I suggest you use only one option.

arguments.ReadFromJSONString(result.pData);
{
if (arguments.ReadJSONString(result.pData))
arguments.PushArguments(arguments);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this line do? arguments.PushArguments(arguments) looks strange

@znvjder
Copy link
Contributor Author

znvjder commented Jan 14, 2024

I'd advocate moving rapidjson to a submodule

It looks like an universal way to make dependency. I like this idea

Pros:

  • Updates are much easier.

  • You can see changes in the submodule from your git client.

  • It's much easier to updated submodule

  • Less premake5 scripts. So we can switch to cmake in the future.

  • Download speed is better for updates

Cons:

  • Slower when repo is used first time.

  • 3d-party repo can be vandalized/deleted. This point is valid for premake modules too.

  • CI/CD should be prepared for this.

  • Submodules can be used only for git submodules

  • Can't be used for prebuilded libraries (CEF)

rapidjson is not under development ( last commit was 2 years ago ). I think we can keep sources in our repo.

@znvjder you have sources in the repo and you download them at the same time. I suggest you use only one option.

Generally, Git submodules are a matter of debate. They have their pros and cons. Certainly, for individuals who are inexperienced or starting their journey with coding and compiling project, it can cause a lot of trouble.

The rapidjson repository is forked at this link; it was created two years ago during the creation of a pull request for discord-rpc. In the official rapidjson repository, the last commit was a month ago.

I’d like us to make decisions about submodules together.

@Pirulax
Copy link
Contributor

Pirulax commented Jan 14, 2024

Keeping sources in this repo makes PRs like this a mess.
There was some Discussion about them over at DC, but I can't find it anymore.

@znvjder
Copy link
Contributor Author

znvjder commented Jan 15, 2024

I’m aware that reviewing such a pull request with attached sources can be problematic and challenging to preview. I’ll try to move rapidjson to submodules today if I can and if permissions allow.

Additionally, I’d like someone to check the permissions in advance because I can’t respond to reviews as they go into a pending status.

@Pirulax
Copy link
Contributor

Pirulax commented Jan 15, 2024

I'd have to fork it into the mta org first I think, because premake has to be added.
But wait for the response of other members too, see what they think about using a submodule.

@TheNormalnij
Copy link
Contributor

I'd have to fork it into the mta org first I think, because premake has to be added.
But wait for the response of other members too, see what they think about using a submodule.

We have clone of rapidjson repository in MTA project already.
I think, premake must be added in the mta-blue repo and the original therapidjson repo must remain untouched. This significantly helps us with updates.

@TheNormalnij TheNormalnij reopened this Mar 3, 2024
@Nico8340
Copy link
Contributor

Nico8340 commented Apr 7, 2024

Any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants