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

Core: limit parallel APContainer writing #2443

Merged
merged 2 commits into from
Nov 13, 2023
Merged

Conversation

Berserker66
Copy link
Member

What is this fixing or adding?

limits how many threads are trying to write files to something reasonable.

How was this tested?

just quickly with LttP to see that it works, did not measure time impact.

If this makes graphical changes, please attach screenshots.

@ThePhar ThePhar added is: refactor/cleanup Improvements to code/output readability or organizization. affects: core Issues/PRs that touch core and may need additional validation. labels Nov 10, 2023
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.

From my testing, bsdiff4.diff of two 4MB files allocates ~60MB of RAM. That is 3 bz2 streams + what the diffing needs. So my initial guess was way off and this change has an even bigger impact.

It fixed the 1000 player SoE gen for me (would've been >60GB of RAM before, and now isn't) while loading all but 1 CPU core to the brim. Having one more thread (n+1) does not give a better result, probably because it will introduce more cache misses.

Gen takes 4sec (3%) longer with this change compared to limiting the number of threads in the pool, which is kind of expected with 1000 threads spinning on a semaphore.

We should leave a comment that there is a better way to fix it (see suggestion below).

The one "bug" this introduces is

Took 45.45132064598147 seconds in SoEWorld.generate_output for player 998, named Player998.

because the slot's generate_ouput now has to wait 44 seconds on the semaphore.

worlds/Files.py Outdated Show resolved Hide resolved
Co-authored-by: black-sliver <[email protected]>
@Berserker66 Berserker66 merged commit ef7d8a6 into main Nov 13, 2023
20 checks passed
@Berserker66 Berserker66 deleted the core_apcounter_semaphore branch November 13, 2023 05:46
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation. is: refactor/cleanup Improvements to code/output readability or organizization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants