1.13(+) server tick logic is flawed

The bug

The server tick logic (MinecraftServer#run()) was changed in one of the 1.13 snapshots or pre-releases. The new logic appears to be faulty (the previous was faulty as well though, see MC-121196).
In the section "Suggested fix" below is a suggested fix and I would strongly advise to use it or at least to use parts of it.

1.13-pre7

long l2 = k.b() - this.aa;
if (l2 > 2000 && this.aa - this.Q >= 15000) {
    long l3 = l2 / 50;
    h.warn("Can't keep up! Is the server overloaded? Running {}ms or {} ticks behind", (Object)l2, (Object)l3);
    this.aa += l3 * 50;
    this.Q = this.aa;
}
this.v();
this.aa += 50;
while (k.b() < this.aa) {
    Thread.sleep(1);
}
this.P = true;

1.13-pre7 deobfuscated

long backlogMillis = currentMillis() - this.lastTickExpectedEnd;

if (backlogMillis > 2000 && this.lastTickExpectedEnd - this.lastWarnTime >= 15000) {
    long backlogTicks = backlogMillis / 50;
    h.warn("Can't keep up! Is the server overloaded? Running {}ms or {} ticks behind", (Object)backlogMillis, (Object)backlogTicks);
    this.lastTickExpectedEnd += backlogTicks * 50;
    this.lastWarnTime = this.lastTickExpectedEnd;
}

this.tick();
this.lastTickExpectedEnd += 50;

while (currentMillis() < this.lastTickExpectedEnd) {
    Thread.sleep(1);
}

this.isServerRunning = true;

It appears the line containing the backlog logic is incorrect, it should subtract the backlog ticks. Otherwise a server already running behind will sleep even longer, is should be:

this.lastTickExpectedEnd -= backlogTicks * 50;

Assuming that is the case, there are still the following problems:

• Possible problems when nanoTime() (used for currentMillis()) overflows; the Java docs suggest to perform comparisons as "t1 - t0 < 0, not t1 < t0"

• Thread.sleep(1) might switch control too often to the server thread even if it still has not reached lastTickExpectedEnd yet

• The server is not able to catch up after running behind without accumulating the backlog to 2000 milliseconds, because only then the lastTickExpectedEnd is adjusted; and always 50 milliseconds are added to the expected end

• It always adds 50 milliseconds to the expected end, even if the backlogTicks had an remainder (this is related to the point above, not being able to catch up)

• The lastWarnTime depends on the tick speed instead of directly using currentMillis()

Suggested fix

The following suggested fix should address all issues mentioned above. Additionally it runs backlog ticks in a loop and does not query the current millis for each iteration then.

Feel free to use it; the author does not have to be mentioned.

At the moment this fix is only theoretical and not tested yet.

Field changes:

• Remove lastTickExpectedEnd

• Add new field this.lastTickEnd, initialized with currentMillis()

Two new local variables outside the while (this.serverRunning) loop are needed:

• backlogMillis = 0

• static maxTickLengthMillis = 50

The following is the content of the while (this.serverRunning) loop:

backlogMillis += maxTickLengthMillis;

while (backlogMillis >= maxTickLengthMillis) {
    backlogMillis -= maxTickLengthMillis;
    tick();
    this.serverIsRunning = true;
}

long currentMillis = currentMillis();
long remainingMillis = maxTickLengthMillis - (currentMillis - this.lastTickEnd) - backlogMillis;

if (remainingMillis > 0) {
    Thread.sleep(remainingMillis);
    this.lastTickEnd = currentMillis + remainingMillis;
    backlogMillis = 0;
}
else {
    this.lastTickEnd = currentMillis;
    backlogMillis = -remainingMillis;
    // Position to show warnings; use currentMillis as time value
}

Notes

• The field isServerRunning can probably be removed since it looks like it is not used anymore

• The watchdog code has to be changed; whatever is used to signal that the server is still running, it has to be in the loop containing the call to tick()