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

Factorio: use orjson #1809

Merged
merged 3 commits into from
Sep 30, 2023
Merged

Factorio: use orjson #1809

merged 3 commits into from
Sep 30, 2023

Conversation

Berserker66
Copy link
Member

What is this fixing or adding?

trial run of the orjson json library in Factorio

How was this tested?

running Generate with Factorio involved.

If this makes graphical changes, please attach screenshots.

@black-sliver
Copy link
Member

to sum up what i said in discord, i see two things:

  1. on current main output is not stable, making the orjson change hard to test. it looks like it places the same items in the same spheres, but at different locations
  2. orjson dicts look like they are not ordered, but sorted by something like hash(key). Not sure if that happens when reading or writing, but in case of factorio it should not be a problem, i would like to tested that though once 1. is solved

@ThePhar ThePhar added the is: refactor/cleanup Improvements to code/output readability or organizization. label May 31, 2023
@Berserker66
Copy link
Member Author

  1. should be solved by now

Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

Generated spoiler for the same seed is identical between main and orjson.

Speed Improvement: 43% for all of factorio

$ python -m timeit 'import orjson; [orjson.loads(open(f"data/{f}.json").read()) for f in ("fluids","items","machines","recipes","resources","techs")]'
1000 loops, best of 5: 258 usec per loop
$ python -m timeit 'import json; [json.loads(open(f"data/{f}.json").read()) for f in ("fluids","items","machines","recipes","resources","techs")]'
500 loops, best of 5: 454 usec per loop

However it should be noted that, as long as we only use it for factorio, the switch has a negative impact:

  • it is not builtin (i.e. not shared between different venvs), using up cache and taking longer1 to load
  • it uses ~10MB of RAM, which is a fair trade-off if we use it in more places
$ python -m timeit 'import orjson; orjson.loads(open("data/fluids.json").read())'
20000 loops, best of 5: 10.3 usec per loop
$ python -m timeit 'import json; json.loads(open("data/fluids.json").read())'
20000 loops, best of 5: 11.8 usec per loop

EDIT: the 10MB figure includes all of its dependencies, so the actual impact is smaller. If we use fragments, we also win back (some) of the extra memory.

Footnotes

  1. loading a single, small file gives almost no improvement:

@black-sliver
Copy link
Member

Re EDIT above: I remeasured
python -c "import NetUtils; import orjson; orjson.loads('{\"1\":123456}')
vs.
python -c "import NetUtils; import json; json.loads('{\"1\":123456}')
and the difference is 7.2MB

@Berserker66 Berserker66 merged commit 58b696e into main Sep 30, 2023
19 checks passed
@Berserker66 Berserker66 deleted the factorio_orjson branch September 30, 2023 21:59
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
* Factorio: use orjson

* Factorio: update orjson

---------

Co-authored-by: black-sliver <59490463+black-sliver@users.noreply.github.com>
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
* Factorio: use orjson

* Factorio: update orjson

---------

Co-authored-by: black-sliver <59490463+black-sliver@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: refactor/cleanup Improvements to code/output readability or organizization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants