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

Async Chunk Loading and Generation #1397

Merged
merged 73 commits into from Sep 26, 2018

Conversation

@aikar
Copy link
Member

aikar commented Sep 1, 2018

Async Chunk Loading and Generation - BETA

STAGE:
STABLE: Should be safe for production. No known bugs at this time. Nearing ready for merge
Custom World Gens are now supported. Conversion from pre 1.13 worlds is supported and async!

BETA TESTERS:
Latest builds of this will be provided (as Paperclip patchers) here: https://aikar.co/async.jar

DISCLAIMER:
Again this is beta - please ensure you have backups.

These builds are now to the point they should be safe to run in production.
There has not been any reported issues yet of any data loss.

However, bugs could lead to server crashes, but they should mostly be worked out now.

TODO LIST:
SEE: https://github.com/PaperMC/Paper/projects/2

DONATE:
Lots of effort has went into making this happen. Want to say thanks and donate?
Foo

aikar added some commits Sep 1, 2018

Async Chunk Loading and Generation
This brings back parity to 1.12 and older versions in that any
chunk requested as part of the PlayerChunkMap can be loaded
asynchronously, since the chunk isn't needed "immediately".

The previous system used by CraftBukkit has been completely abandoned, as
mojang has put more concurrency checks into the process.

The new process is no longer lock free, but tries to maintain locks as
short as possible.

But with 1.13, we now have Chunk Conversions too. A main issue about
keeping just loading parity to 1.12 is that standard loads now
are treated as generation level events, to run the converter on
another thread.

However mojangs code was pretty bad here and doesn't actually provide
any concurrency...

Mojangs code is still not thread safe, and can only operate on
one world per thread safely, but this is still a major improvement
to get world generation off of the main thread for exploration.

This change brings Chunk Requests triggered by the Chunk Map to be
lazily loaded asynchronously.

Standard chunk loads can load in parallel across a shared executor.

However, chunk conversions and generations must only run one per world
at a time, so we have a single thread executor for those operations
per world, that all of those requests get scheduled to.

getChunkAt method is now thread safe, but has not been tested in
use by other threads for generations, but should be safe to do.

However, we are not encouraging plugins to go getting chunks async,
as while looking the chunk up may be safe, absolutely nothing about
reading or writing to the chunk will be safe, so plugins still
should not be touching chunks asynchronously!
Replace synchronization on the region loader
This blocks generation threads a lot due to saving operations and other stuff

We just need to ensure 2 things don't hit the progress cache at same time
Post chunk to the world on main thread only
This may be the the completion of this feature... time to test hardcore!
@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 2, 2018

build to jar is updated with latest updates

There is still an issue with entities created during world generation though thats going to be tough to resolve due to bad logic on mojangs part

Implement a super dirty but reliable hack to stop async entity adds
I feel so dirty, but this is the only reliable way to do it without
rewriting all the code or waiting for mojang
@hugmanrique

This comment has been minimized.

Copy link
Contributor

hugmanrique commented on Spigot-Server-Patches/0358-Async-Chunk-Loading-and-Generation.patch in 9097d14 Sep 2, 2018

Ok, I'm a bit of a noob with MT-safe code, but the pendingChunks field is a synchronized map (via Long2ObjectMaps.synchronize), wouldn't this mean atomic reads and writes for #get and #put?

The javadocs for that method redirect to Collections.synchronizedMap, which specifies synchronization is only needed when you are iterating through any of the map views (to avoid CMEs I assume).

Is there something I'm missing? Thanks in advance

This comment has been minimized.

Copy link
Contributor

Trigary replied Sep 2, 2018

The synchronized block would not be necessary if there was only one read/write. In that block, there is always 1 read and 1 or 0 writes. This means that if the synchronized block wasn't there, then the object at the key key could be assigned in another thread and then that assignment would be overridden in this thread, etc.

This comment has been minimized.

Copy link
Contributor

hugmanrique replied Sep 2, 2018

Ah, thanks for the explanation 😄

aikar added some commits Sep 2, 2018

Many major fixes, post level loading to main
major changes to ensuring all chunk posting stuff occurs on main.

this fixes a few issues
@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 3, 2018

New beta build up!

All known bugs should now be fixed. I need testers to find bugs!
But please, test on a COPY of your production world, do not run this on your live server until it looks like no more major bugs are seen.

aikar added some commits Sep 4, 2018

Merge branch 'pre/1.13' into async-chunks
* pre/1.13:
  Fix concurrency issues in DataFixers
  [CI-SKIP] Download mojang libraries sources so we can modify them
  Add ray tracing methods to LivingEntity (#1410)
  Check that phantom spawned uuid is set for save/load
  [CI-SKIP] Improve last patch to not use wildcard
  [CI-SKIP] Fix the NMS Imports detecting our own files
  [CI-SKIP] [Auto] Rebuild Patches
  Remove Pass World Tile Entities patch
  fix typo in patch file
  [CI-SKIP] [Auto] Rebuild Patches
  Add Force-Loaded Chunk API (#1387)
  Cached, local-class-supporting task names (#1409)
  [Auto] Updated Upstream (Bukkit/CraftBukkit/Spigot)
  Make CraftWorld#loadChunk(int, int, false) load unconverted chunks (#1407)
  [Auto] Updated Upstream (CraftBukkit)
Remove synchronization from GameProfileSerializer
DataFixers are now about to be thread safe on next merge
Merge branch 'pre/1.13' into async-chunks
* pre/1.13:
  Fix another case of concurrency issue in data fixers
Merge branch 'pre/1.13' into async-chunks
* pre/1.13:
  Fix yet another issue with concurrency with datafixers
@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 4, 2018

New build uploaded with even better performance for chunk conversions, as I've solved hopefully all the concurrency concerns with datafixers.
Also made spawn chunks load async.

aikar added some commits Sep 5, 2018

Merge branch 'pre/1.13' into async-chunks
* pre/1.13:
  MC-2025 - Save entity AABB to prevent fp wobble
  [Auto] Updated Upstream (Bukkit)
  [Auto] Updated Upstream (CraftBukkit)
Replace cached executor with custom priority queue executor
CachedThreadPool uses a SynchronousQueue which could block main
some to deliver it to a thread.

The cached pool also was unbounded, but being bounded would be bad
since the queue is synchronous, we could block main.

This commit implements a custom executor (havent fully implemented
ExecutorService yet though), that allows us to deliver work to
a queue that can be processed later.

The submit task returns an object that you can obtain a future
to know when the task is done.

This custom executor also offers a concept of priority.

We can schedule an async load as normal priority, and then if
that chunk is all of a sudden needed as a blocking request on the
main thread, we can add it to the high priority queue.

high priority queue will always be processed first, so this lets
us reduce any main thread blocking operation quite quickly
by pushing it to the front of the line.
Merge branch 'pre/1.13' into async-chunks
* pre/1.13:
  Updated Upstream (CraftBukkit)
@andris155

This comment has been minimized.

Copy link

andris155 commented Sep 7, 2018

@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 7, 2018

@andris155 does it happen consistently? Thanks that's helpful

@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 7, 2018

Note it was due to a random teleport

@andris155

This comment has been minimized.

Copy link

andris155 commented Sep 7, 2018

Sometimes when players use Randomteleport sign. I tested other randomteleport plugin, but same error.

@andris155

This comment has been minimized.

Copy link

andris155 commented Sep 7, 2018

Couldn't generate chunk (plotworld:19,-16) error (Generator: PlotSquared)
https://pastebin.com/2rzNdkk6

@aikar aikar added this to In Progress in Async Chunks Sep 9, 2018

@aikar aikar added this to In progress in Performance Improvements Sep 9, 2018

Merge branch 'pre/1.13' into async-chunks
* pre/1.13:
  Remove no longer needed tests due to last change
  Remove deadlock risk in firing async events
  Fix invalid data types for particles and fix colors in the ParticleBuilder (#1422)
  Update again so the dumb bot stops breaking things
  [Auto] Updated Upstream (Bukkit/CraftBukkit)
  Improve death events (#1362)
  [CI-SKIP] [Auto] Rebuild Patches
  Fix #1420 (#1421)
  [Auto] Updated Upstream (CraftBukkit)
  [Auto] Updated Upstream (Bukkit/CraftBukkit)
@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 22, 2018

Updated with fixes for jordans issue.

@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 23, 2018

@bergerkiller got reports of errors with one of your plugins: https://pastebin.com/shyhZMV8

Can you please add support for this to your plugin.

@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 23, 2018

Oh I wonder if its because I made the ctor package-private, let me bump that up.

Improve compat with plugins that extend CPS
No guarantees this will work, but its an attempt.
@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 23, 2018

user reported its no longer complaining when i made the class public now.

Merge branch 'master' into async-chunks
* master:
  [Auto] Updated Upstream (CraftBukkit)
  [Auto] Updated Upstream (CraftBukkit)
  Fix NPE race condition in ServerListPingEvent
  [CI-SKIP] Add copy of my checkout-pr script for other team members
  Create API for CanPlaceOn and CanDestroy NBT tags (#1015)
  Expose attack cooldown methods for Player (#1412)
  Add creative-arrow-despawn-rate (#1411)
  Add obfuscation helper (aikar's request) (#1468)
@bergerkiller

This comment has been minimized.

Copy link

bergerkiller commented Sep 23, 2018

If there happen to be issues after these changes with my plugin(s) people can open an issue ticket over at https://github.com/bergerhealer/BKCommonLib

I'm close to releasing a new 1.13.1-v2 with mostly just bugfixes and compatibility patches, so any extra changes needed for supporting these changes in paper are no problem. Just need to know about it timely :)

Class / member visibility tends to be an issue with generated code, shame the error was so cryptic though. Indeed, your changes should fix that.

aikar added some commits Sep 23, 2018

Merge remote-tracking branch 'origin/master' into async-chunks
* origin/master:
  Enable experimental ASM support for Java 11 plugins (#1480)
  [CI-SKIP] Add more detailed information for plugin devs
Merge branch 'master' into async-chunks
* master:
  Updated Upstream (CraftBukkit/Spigot)
  Make bad custom name resolution more specific
@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 24, 2018

build updated with latest master as well as a new concurrency fix. update!

@WesleyVanNeck

This comment has been minimized.

Copy link

WesleyVanNeck commented Sep 25, 2018

awsome it works great i dident found a big bug or small one

@Lachney

This comment has been minimized.

Copy link

Lachney commented Sep 25, 2018

awesome work on all of this, keep it up @aikar

@bergerkiller

This comment has been minimized.

Copy link

bergerkiller commented Sep 25, 2018

@aikar One suggestion for the future: considering Paper changes the chunk provider server, and BKCommonLib does so too (using a hook) passing to the underlying implementation, I have a request to add something in paper so that this hooking is no longer required.

BKCommonLib hooks into the server's CPS to override and handle getMobsFor. If paper can provide a similarly suited event for plugins to handle this, I can disable this hooking entirely, avoiding potential compatibility issues in the future with these enhancements.

The getMobsFor event handling enables a plugin to tweak, disable or alter what mob groups spawn on the worlds. This is an early stage of spawning entities. By doing this here, a significant performance improvement is achieved, because no entities have to be created - and then destroyed - as part of the normal spawn events Bukkit offers.

Would you be interested in providing such extra functionality to plugins? I will write a separate issue ticket for this feature request if it has your initial approval, to stay on topic of this issue ticket.

(BKCommonLib extends the CPS presently installed, so it should not cause any of these changes in paper to be cancelled out)

aikar added some commits Sep 25, 2018

Merge remote-tracking branch 'origin/master' into async-chunks
* origin/master:
  Honor EntityAgeable#ageLock (#1481)
  Avoid dimension id collisions (#1487)
  [Auto] Updated Upstream (CraftBukkit)
  Allow zero revive health when it matches maxHealth
  [Auto] Updated Upstream (Bukkit/CraftBukkit)
  [Auto] Updated Upstream (Bukkit)
  [Auto] Updated Upstream (Bukkit/CraftBukkit)
Merge branch 'master' into async-chunks
* master:
  Re-enable light queue toggle, optimize neighbor checks, add max queue time
@mibby

This comment has been minimized.

Copy link

mibby commented Sep 26, 2018

@aikar Does https://aikar.co/async.jar auto-build and host the latest changes made to the async branch? The file didn't seem to change with the new master commit merged.

@MicleBrick

This comment has been minimized.

Copy link
Contributor

MicleBrick commented Sep 26, 2018

It does not auto update, you need to redownload it

@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 26, 2018

It is now updated @mibby. I had internet issues last night and could not access my server. But rebuilt and uploaded now with the light queue changes

@aikar aikar merged commit c5fbde9 into master Sep 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Async Chunks automation moved this from In Progress to Done Sep 26, 2018

@aikar

This comment has been minimized.

Copy link
Member Author

aikar commented Sep 26, 2018

Thank you to everyone who has helped test this! We have now landed this in master!

Paper builds 302+ now include Async Chunks. Please report issues as a new issue and on discord

@aikar aikar deleted the async-chunks branch Sep 26, 2018

@aikar aikar added feature and removed in-progress labels Sep 27, 2018

@BBoyJD10

This comment has been minimized.

Copy link

BBoyJD10 commented Sep 27, 2018

Sometime citizens npcs do not show

@sgdc3

This comment has been minimized.

Copy link
Contributor

sgdc3 commented Sep 28, 2018

@BBoyJD10 i noticed that too but seems unrelated to the async chunk change, as it was happening before the merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.