search

Cog-Creators / Red-DiscordBot

A multi-function Discord bot
https://docs.discord.red
GNU General Public License v3.0
4.76k stars 2.3k forks source link

Switch to orjson, fallback to stdlib json #6387

Open japandotorg opened 3 months ago

japandotorg commented 3 months ago

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

Jackenmen commented 3 months ago

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 commented 3 months ago

yikes, well that's a bummer.

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

Jackenmen commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.