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

Change: [Network] Remove now defunct save game transfer packet limiter #9108

Merged

Conversation

@rubidium42
Copy link
Contributor

@rubidium42 rubidium42 commented Apr 25, 2021

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')
Copy link
Member

@TrueBrain TrueBrain left a comment

Less code \o/ More depending on OSes \o/

Tnx for this :)

As mentioned on IRC, we have to keep an eye on timeouts, as they might not hit earlier, but that part is due for a refresh anyway! So looks-good-to-me!

@James103
Copy link
Contributor

@James103 James103 commented Apr 25, 2021

Commit b721787 caused the Japanese translation of STR_ROAD_TOOLBAR_TOOLTIP_CONVERT_ROAD to become malformed, causing all builds to fail as a result.

@rubidium42 rubidium42 force-pushed the remove_savegame_transfer_limit branch from b649010 to ca77ce3 Apr 25, 2021
@rubidium42 rubidium42 merged commit 5afb090 into OpenTTD:master Apr 25, 2021
12 checks passed
@rubidium42 rubidium42 deleted the remove_savegame_transfer_limit branch Apr 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants