Paper: Server overloaded with Citizens and latest Paper

[MrMaleficus]

What behaviour is expected:

The server works fine (good tps)

What behaviour is observed:

Here a spark sample : https://spark.lucko.me/#QoZSEld7dF
The server is very laggy and overloaded

Steps/models to reproduce:

Create a few NPCs with the latest version of Citizens and Paper on a Minecraft Map with a few player around and wait for the "Server is overloaded" message

Plugin list:

Plugins (51): AdvancedRegionMarket, AncientGates, BlocksHub, BottledExp, Brewery, BuycraftX, Citizens, CoreProtect, CrazyCrates, Denizen, Depenizen, dynmap, Dynmap-Towny, Essentials, EssentialsChat, EssentialsSpawn, FastAsyncWorldEdit, HolographicDisplays, Jobs, JSONAPI-RELOADED, LibsDisguises, LuckPerms, LWC, MarriageMaster, MDCQuestioner, Minepacks, MiniaturePets, MovingDevApi, Multiverse-Core, OpenInv, PCGF_PluginLib, PlaceholderAPI, PluginConstructorAPI, PotionStacker, ProtectionLib, ProtocolLib, PureTickets, RewardPRO, RFChairs, Shopkeepers, spark, SuperTrailsPro, Towny, TownyChat, TownyNameUpdater, TreasureChestReloaded, Vault, VoxelSniper, WorldBorder, WorldEdit, WorldGuard

Paper build number:

.... [20:08:15 INFO]: This server is running Paper version git-Paper-353 (MC: 1.15.2) (Implementing API version 1.15.2-R0.1-SNAPSHOT)
.... [20:08:15 INFO]: Previous version: git-Paper-335 (MC: 1.15.2)
.... [20:08:15 INFO]: You are running the latest version

Anything else:

With paper-335 there is no issue.

[Momonche]

With paper-343 there is no issue too

[mcmonkey4eva]

For clarity, the relevant Citizens code is an public void onChunkLoad(ChunkLoadEvent event) event with Bukkit.getScheduler().scheduleSyncDelayedTask(runnable) where the runnable loops over a list of NPCs meant to be in that chunk and spawns them into place.

At a guess of what would've changed, it's likely something to do with chunks loading/unloading rapidly, causing the ChunkLoadEvent to fire rapidly and therefore all associated spawning code to fire rapidly.

I was pinged in the Paper discord with some discuss about the slower parts of the spawn sequence (unnecessary construction and processing of packets that will go to nowhere), and while fixing that up would certainly be good in general and would mitigate this issue, it wouldn't actually fix this issue. (ie, reducing the lag impact from chunks unloading and reloading rapidly wouldn't actually fix the issue of chunks unloading and reloading rapidly, it just makes people a bit less likely to notice it).

[TheoRetisch1]

I'm facing the same problem, build 345 works fine :)

[aikar]

Can you try with 355?

[Momonche]

Just test with paper-365 and still have an issue

https://timings.aikar.co/?id=9a78d03646ee40e288e31baeadb347db#timings

[MrMaleficus]

Same, test with latest, there is the same issue : https://spark.lucko.me/#mmK3dRTU7o

[Techcable]

@MrMaleficus Thanks for the profiling report. Citizens is spending 62% of profiler's time in the task that runs respawnAll.

The main cost is calling nms.WorldServer.addEntity (57%) -> paper.util.misc.AreaMap.add (55%) -> nms.PacketPlayOutMapChunk.sendChunk (54%)

I would think this would be significantly improved over vanilla behavior with our patches to distance maps...........
Specifically b4e629a283c851cec0305ccf9e4d9edb067767d8 and 64cfcf3e4b16148cdc352d58a29e3428715b1157 should avoid this issue.

This could be because citizens is frequently spawning/despawning NPCs every time a chunk is loaded/unloaded.

Every npc they spawn triggers a call to addEntity, which sends chunk updates to all nearby players. Assuming the updates aren't somehow batched, that's numNpcs * numPlayers updates on each load/unload?

This seems more like an interaction with Citizens, than a bug in paper itself. Should we be queuing these updates?

I'll investigate further once I get back to my laptop.

[Proximyst]

@Techcable The easiest (and perhaps even best in certain cases?) fix would be to allow player entities to specify whether they'd like to accept chunk packets. If they don't, don't construct them - that's the expensive part.

These two together should manage it, but I'm unsure if they're necessary with the patches you've mentioned. I am therefore holding off with a PR, but if it appears this is an issue forwards, I will happily open one (not that a 2 line diff is hard to port by someone else anyways):
https://github.com/Proximyst/Paper/blob/toggle-chunk-sends/Spigot-Server-Patches/0548-Allow-fake-players-to-deny-chunk-packets.patch
https://github.com/Proximyst/Citizens2/blob/master/v1_15_R1/src/main/java/net/citizensnpcs/nms/v1_15_R1/entity/EntityHumanNPC.java#L94-L98

In my implementation, plugins will have to opt in for performance improvements, but it's also doable to default the boolean to getClass() != EntityPlayer.class and its derivatives.

[Techcable]

@Proximyst Ahhh, so the problem is we're actually trying to send updates to the NPCs?

I think something like your second solution seems better. We could just do an getClass check or detect whether the playerConnection actually points to a real connection 😄

I really don't see the harm in making a PR right now. The patches I mentioned just filter out players based on distance, without considering whether or not they are NPCs

[Proximyst]

Certain plugins (apparently? Aikar said something about this on Discord) use a networking handler for capturing packets using fake players. I'm sure they're abusing the wrong mechanic, but it's their choice.

If the general concensus is that this one use-case can do what I do in my Citizens fork to ensure they receive the packets, I'm sure it's no problem to just check their player connection.

[mcmonkey4eva]

Again, to be clear: while the pointless packet sending is a nuisance that should be fixed, it is NOT the fundamental issue within this bug report, which is that in recent builds of Paper specifically, chunks are rapidly loading and unloading, which in Citizens means NPCs rapidly spawning and despawning.
Fixing the pointless packet processing now runs the risk of hiding this issue without fixing it.

[HexedHero]

Having the same lag causes with my personal plugins that are loading data on chunk load due to the new amount of chunk loads/unloads bringing the server to its knees.

[PhanaticD]

shouldn't the plugin just check if the chunk is still loaded once the task runs? though it does seem weird chunks are loading then unloading in just a tick

[MrMaleficus]

@aikar The issue is not fixed :(
I have tried with Paper-370 : https://spark.lucko.me/#zkZh4wAxwW

Edit : the issue seems to be fixed if i turn the always-keep-loaded chunks config from Citizens, but that's just hiding the issue i think.

[aikar]

only thing i can suggest is try removing denizens for testing purposes to see if its triggering a reload. I wonder if you got some denizen script that is triggering a reload after a delayed task.

no one can reproduce this, not even citizens devs.

[CapraGenus]

Still getting this issue, running paper-77 (1.16.1), after about 5/6 players are on the server starts to drop TPS and lags hard. Timings attached

https://spark.lucko.me/#f9Otq5miVV

[MaDt0m]

I'm getting this exact same issue as others. Extreme tps lag caused by citizens

[CapraGenus]

This issue can be alleviated by enabling always-keep-loaded within citizens as it stops the chunk spawn loop occurring by keeping any chunks with NPCs in loaded, but that is only glossing over the issue itself.

[aikar]

I'm going to need someone to provide me a copy of your plugins folder and maybe affected chunks files, as well as the config files to reproduce this.

If you want, you can privately email me it, email @ https://github.com/PaperMC/Paper/commit/29453f1dbeb10812aad3280d0c2b7883084426a0.patch

[CapraGenus]

I can send you a copy now if you provide an email :P

[FranKusmiruk]

I can send you a copy now if you provide an email :P

the patch provides an email ([email protected])

[BillyGalbreath]

I can send you a copy now if you provide an email :P

the patch provides an email (-redacted-)

GFJ, you just handed the email to web crawlers everywhere on a silver platter. smh

[fullwall]

I've pushed an update to Citizens which may address this.

[Momonche]

Just test with Paper 378 1.15.2 and Citizens 2049 and TPS is ok
But this spam my console :

And if I use /npc tp on it chunck never load

[Momonche]

On server stop Citizens spam this too

[fullwall]

The second error is off topic (Spigot bug)

[Blueeyestar]

Is the problem still there or has it been fixed. would like to use paper again because it is better than the alternative in all things

[aikar]

It was reported by fullwall that it was actually a Citizens problem that Spigot is just "masking", and that it should be fixed in the newest Citizens build.

I'm just leaving this open until we get some confirmation.

The reason I could never reproduce it was because my citizens build was too old before said bug.

[Blueeyestar]

that would be nice if it would work again then I will go back to paper with current citizens with the latest update and watch and report my experience tomorrow

[Blueeyestar]

I have now run the server for three hours with Citizens and paper.
So far no abnormalities have appeared.
I will continue testing until tonight and bring an update on performance

Server problems 2 days ago with the old paper and citizens files in 1.16.1 with 0 players

Server today with the latest paper (# 87) and latest citizens (Build # 2053) at 0 players. The server is quiet. No rashes are noted

[aikar]

woot :) Going to close this then.

Anyone who was having this issue, update your citizens build.