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

Switch to orjson, fallback to stdlib json #6387

Open
wants to merge 9 commits into
base: V3/develop
Choose a base branch
from

Conversation

japandotorg
Copy link
Contributor

Description of the changes

To optimize performance we should utilize orjson, which is already included in requirements/base.in, but unused in Red.

I have been using a modified version of Red with orjson (replacing stdlib json, no fallback) without any problem and have noticed performance gains so far, I primarily used orjson.dumps and orjson.loads, while other methods like json.dump, json.load, aiohttp.ClientSession(json_serialize=...) remained unchanged using stdlib json, also never in audio before.

While I don't think a full switch to orjson is necessary, but I think we should give orjson a chance.

Have the changes in this PR been tested?

No

@github-actions github-actions bot added Category: Core - API - Config This is related to the `redbot.core.config` module and `redbot.core.drivers` package. Category: Cogs - Audio This is related to the Audio cog. Category: Cogs - Streams This is related to the Streams cog. Category: Cogs - Downloader This is related to the Downloader cog. Category: Core - API - Utils Package This is related to stuff in `redbot.core.utils` Category: Meta This is related to the repository maintenance. Category: Core - API - Other This is related to core APIs that don't have a dedicated label. Category: Core - Command-line Interfaces This is related to Red's CLIs (redbot, redbot-launcher, redbot-setup). Category: Core - Other Internals This is related to core internals that don't have a dedicated label. labels Jun 9, 2024
@Jackenmen
Copy link
Member

It raises JSONEncodeError on an integer that exceeds 64 bits by default or, with OPT_STRICT_INTEGER, 53 bits.
Source: https://github.com/ijl/orjson?tab=readme-ov-file#serialize

This makes it unsuitable for Config. Additionally, the support for dataclasses and other types would make it harder to migrate back.

@japandotorg
Copy link
Contributor Author

yikes, well that's a bummer.

Is there anything that can be done? or maybe an alternative?

@Jackenmen
Copy link
Member

Nothing can really be done about this in the current Config API. If there's ever a new one, probably, though I don't know if a new one would even have a JSON driver in the first place.

You could probably sprinkle some orjson usage in other places in Core, just not anything to do with Config. I don't know if it will really give any significant gains though, I don't think we use JSON outside of Config much.

Other than that, I glanced at the diff and I just want to say that there's no point in the _json module - orjson is not an optional dependency of Red.

@japandotorg
Copy link
Contributor Author

ah I see

You could probably sprinkle some orjson usage in other places in Core, just not anything to do with Config. I don't know if it will
really give any significant gains though, I don't think we use JSON outside of Config much.

for now I'll just look into this for this PR, if the org thinks that this is really unnecessary than the PR can be closed

It raises JSONEncodeError on an integer that exceeds 64 bits by default or, with OPT_STRICT_INTEGER, 53 bits.

perhaps can we use a fallback to json incase this happens?

@Jackenmen
Copy link
Member

perhaps can we use a fallback to json incase this happens?

That is an interesting idea though there are more cases where orjson differs in serialization and that could lead to introducing discrepancies between drivers (and even within the driver in case of the fallback) - for example it serializes UUID types while normal JSON would fail at that point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Cogs - Audio This is related to the Audio cog. Category: Cogs - Downloader This is related to the Downloader cog. Category: Cogs - Streams This is related to the Streams cog. Category: Core - API - Config This is related to the `redbot.core.config` module and `redbot.core.drivers` package. Category: Core - API - Other This is related to core APIs that don't have a dedicated label. Category: Core - API - Utils Package This is related to stuff in `redbot.core.utils` Category: Core - Command-line Interfaces This is related to Red's CLIs (redbot, redbot-launcher, redbot-setup). Category: Core - Other Internals This is related to core internals that don't have a dedicated label. Category: Meta This is related to the repository maintenance.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants