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

Fix unnecessary mmap'ing of compass .dat files #648

Merged

Conversation

szuend
Copy link

@szuend szuend commented Dec 31, 2024

Fixes #595.

The access of the compass/*.dat files is currently broken. During chunk generation we easily mmap the same .dat file (1 MB each) thousands of times, leading to OOMs if the VM/OS doesn't unmap fast enough during GCs.

The access to the compass/*.dat files is synchronized via a single "Compass Service" thread. The CompassService class posts job tasks to this thread. The current idea is that when each task finishes, it checks whether more tasks are queued up, and if not, unmap the .dat file. There are two main issues:

  1. The job accounting is plain broken. The CompassServide.#jobSize grows only into the negative (due to some missing jobSize++). This means that we un-map all .dat files after each job even if more jobs are queued up.
  2. If jobs are scheduled a couple milli seconds apart, we'd still un-map and then re-map the .dat file.

This PR fixes this by removing the the broken job accounting all-together and replaces it with a per dimension timeout: If a .dat file goes unused for 60 seconds, we'll unmap it.

Local testing shows that we go from around ~10k map/unmap operations to 4 when generation a new SP world (with boosted meteor generation chances so I don't have to go looking).

Implementation details

The PR achieves this by adding a new AutoClosingCompassReader class. It is associated with a single CompassReader that needs to be retrieved via AutoClosingCompassReader#get. Ever time get is called, we schedule a delayed "closing task". If one is already schedule when get is called, we'll cancel it.

Copy link
Member

@serenibyss serenibyss left a comment

Choose a reason for hiding this comment

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

This looks like a great improvement, thanks!

@serenibyss serenibyss enabled auto-merge (squash) January 3, 2025 06:06
@serenibyss serenibyss merged commit d521efe into GTNewHorizons:master Jan 3, 2025
1 check passed
michaeldoylecs pushed a commit to michaeldoylecs/Applied-Energistics-2-Unofficial that referenced this pull request Jan 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OutOfMemory due to spawnMeteoriteCenter
2 participants