PaperMC / Folia

Fork of Paper which adds regionised multithreading to the dedicated server.
GNU General Public License v3.0
3.32k stars 392 forks source link

Issue with client-desync in relation to server-ticking: #196

Open PedroMPagani opened 4 months ago

PedroMPagani commented 4 months ago

Expected behavior

The expected behavior is so that when the client sends packets, they get processed "immediately", currently what Folia does for every single packet is schedule each packet as a new task, for 1L. If you notice a simple debug, (I have done this) - The time between the packets arrive and get processed should be "instantly" - this is not really a vanilla only thing, but this affects PVP overall and other actions that players would notice a 50ms latency on their action, SPECIALLY if they have a low ping too.

Observed/Actual behavior

Tt really depends on how the player client ticking is also aligned and their ping, while on a paper jar, this won't happen because when the packet arrives it gets processed, the packets are delayed by one tick from when they arrive, and they don't get processed "instantly", they only get processed on the .executeTick() method, this is causing extra delays on actions overall, specially combat in survival.

Steps/models to reproduce

log System Millis of every single packet on the .genericsFtw, and on the ensure same thread that will make the runnable process, observe the behavior of when the .handle method gets called, this will show the latency issue. I also noticed that the latency of processing of the packets, depends on how fast/ or the timing of in which the regions are created (I am not fully sure if this is because of region creation). - I am able to notice a difference upon restarting it though, sometimes less latency, sometimes high latency.

Plugin and Datapack List

No plugins nor data packets

Folia version

[12:28:35 INFO]: This server is running Folia version git-Folia-"8939611" (MC: 1.20.4) (Implementing API version 1.20.4-R0.1-SNAPSHOT) (Git: 8939611 on dev/1.20.4) You are running the latest version Previous version: git-Folia-"30ee81a" (MC: 1.20.2)

Other

No response

PedroMPagani commented 4 months ago

I don't see lots of actual solutions for this, since the new threading is pretty much tasks scheduled and shared threads across regions, I feel like possibly having sub-ticks to process incoming packets on the same region scheduled threads would possibly help. (I implemented this).

DrBotz commented 4 months ago

Yes

Netherwhal commented 2 months ago

This is one of the few issues we still face with Folia in comparison to Paper.

StateException commented 2 weeks ago

I also noticed this. As I understand it, the logic is embedded in the ensureRunningOnSameThread code located in the PacketUtils class.

                gamePacketListener.player.getBukkitEntity().taskScheduler.schedule(
                    (net.minecraft.server.level.ServerPlayer player) -> {
                        run.run();
                    },
                    null, 1L
                );

Why do we need to call packet.handle after 1 tick? Can't we perform this operation immediately? Can someone explain the reason for this behavior? I would like to resolve this issue.

Thanks!

electronicboy commented 2 weeks ago

The earliest you can schedule a task on an entity is for the task to run the next time the entity ticks. You can't perform the operation immediately because you need to run the packet logic within the entities context.

The behavior here is going to be different because on a vanilla server where there is only 1 thread, you don't need to worry about dispatching packet handles into the correct thread context, there only is 1, and so you can just use the shared task queue which allows packets to be dispatched into the thread context which is heavily polled for work by the server.

StateException commented 2 weeks ago

The earliest you can schedule a task on an entity is for the task to run the next time the entity ticks. You can't perform the operation immediately because you need to run the packet logic within the entities context.

The behavior here is going to be different because on a vanilla server where there is only 1 thread, you don't need to worry about dispatching packet handles into the correct thread context, there only is 1, and so you can just use the shared task queue which allows packets to be dispatched into the thread context which is heavily polled for work by the server.

Thank you for the detailed explanation. I would like to clarify... So, logically, if we direct the packet immediately to the correct context before the ensureRunningOnSameThread check is called in ServerGamePacketListenerImpl.handle$Action, we will get the paper/vanilla behavior?

electronicboy commented 2 weeks ago

The only current mechanism for ensuring that you get into that context is the entity scheduler, the thing is that the scheduler can only promise to run a task on the next tick. as regions do not spin wait between ticks, there is no mechanism to "immediately" post a task to run against an entity in a fast manner, and you have to remember that entities are not strictly held to a region, and so anything region based is also going to be precarious

Luuzzi commented 2 weeks ago

The only current mechanism for ensuring that you get into that context is the entity scheduler, the thing is that the scheduler can only promise to run a task on the next tick. as regions do not spin wait between ticks, there is no mechanism to "immediately" post a task to run against an entity in a fast manner, and you have to remember that entities are not strictly held to a region, and so anything region based is also going to be precarious

Thank you for the clear response. Purely hypothetically, we can’t divide these 50ms into subticks (for example, every 1ms) and perform only “urgent” operations in them? Won't this lead to some unexpected behavior or something?

UPD: I understand that there is no such mechanism now, I’m just wondering.

PedroMPagani commented 2 weeks ago

Yes. That's what's being done on DonutSMP. (Not 1ms though, since that's not very CPU-wise and you'll likely need low latency kernels for that as well usually). 200 TPS. 5ms subticks.

Luuzzi commented 2 weeks ago

Yes. That's what's being done on DonutSMP. (Not 1ms though, since that's not very CPU-wise and you'll likely need low latency kernels for that as well usually). 200 TPS. 5ms subticks.

Good solution, glad to hear it works, btw thanks for info.