Motivation / Problem

The "limiting" of the number of packets that can be sent each tick onto a socket is effectively broken. With slow compression the number of packets that it could then dump onto the network stack could be 32768 (~50 MiB) which makes little sense. After all, when compression is slow the packets will arrive slowly and there won't be a moment where that many packets would even be dumped there. Though more importantly, that completely undermines the logic that was once put in place.

Some archeology on the origin of the to be removed code, and the uselessness of it now.
Before the network branch in of 2004 the save game was written to an effectively blocking socket. Effectively as it would just busy loop around send-ing on a non-blocking socket and retry when getting EWOULDBLOCK. Transfer was not limited, it was just blocking the server due to the non-threaded saving directly writing to the network socket.
In the network branch the limiter got added in the same commit that removed the old code; see http://www.icosahedron.de/openttd/git/svn_mirror.git/commitdiff/8769c618f8a982787b627e2b5ed2f4a7d9cbc311.
Back then it might have made some sense because the save game would first be written completely to disk, and then sent to the client. In other words, no limiting would just dump everything at once. Whether that causes problems is unknown as the limiting was added directly without any reasonable explanation in the code or commit message.
Much later saving the save game became threaded, so the packets could be generated more or less in the ancient way; directly create the packets during the saving of the game, however now they are put into an internal queue. This queue is then transferred to the socket queue with the same speed limiting of the original code.
However, the original code was written with the understanding that the packets could be generated instantaneously, and in the amount that it could send. So, if it determined it could send 128 packets, it would create 128 packets and try to send them. If all packets could be sent, it would double that amount, and try with 256 the next tick.
With the save game compression still running the number of packets that would be generated could be way less than the number of packets that could be sent. The result, it determined it could send 128 packets, is gets 2 packets, and tries to send them. All of them could be sent, so the limit would be doubled. The next tick the same happened doubling the limit again. So in effect the limit would have no effect whatsoever, except that it could overflow the limit to 0 stopping the transfer completely (#9106).

So practically this limiter has likely not been used in many years, except potentially causing stalls in the download of the save game which would be described away as network issues. It might be somewhat useful for cases where the server does not use threaded saves, but then the benefit of just dumping everything on the network buffer is that the OS can do its best to get the save game as fast as possible to the client instead of introducing more details by sending 4 packets, then waiting 30 ms, etc.

And, since there is no bug report that caused this logic to be added there is no way to ascertain what the initial reasons were to add the rate limiting.

Description

Removes the whole packet limiter logic as there is no clear reason why it exists.

Change the packet writer to just dump all the packets into the socket, i.e. take the lock once per tick instead of taking the lock for each packet it is going to send onto the network socket.

Limitations

Hopefully none, but that remains to be seen with real users. As such this should to be backported to the 1.11 branch, but rather go through proper half a year of nightlies, the betas and RCs before hitting stable users.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

• The bug fix is important enough to be backported? (label: 'backport requested')
• This PR affects the save game format? (label 'savegame upgrade')
• This PR affects the GS/AI API? (label 'needs review: Script API')
  • ai_changelog.hpp, gs_changelog.hpp need updating.
  • The compatibility wrappers (compat_*.nut) need updating.
• This PR affects the NewGRF API? (label 'needs review: NewGRF')
  • newgrf_debug_data.h may need updating.
  • PR must be added to API tracker