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

Add: only buffer at most 5 max-sized packets per client #20

Merged
merged 1 commit into from May 4, 2020

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented May 4, 2020

This means that when a user downloads a 500+ MiB content entry,
and is on a slow connection, the asyncio write() task will no
longer cache this in full in the memory. Now it will "async" out
till there is room in the write buffer again before continueing
to write. This should heavily reduce the memory footprint in
production.

Additionally, raise an exception if the socket is being closed,
instead of returning False. This means it bubbles up for all
send_packet() calls, which should reduce the amount of warnings
we are now seeing about writing to a closed socket.

Also fixes #15

This means that when a user downloads a 500+ MiB content entry,
and is on a slow connection, the asyncio write() task will no
longer cache this in full in the memory. Now it will "async" out
till there is room in the write buffer again before continueing
to write. This should heavily reduce the memory footprint in
production.

Additionally, raise an exception if the socket is being closed,
instead of returning False. This means it bubbles up for all
send_packet() calls, which should reduce the amount of warnings
we are now seeing about writing to a closed socket.
@TrueBrain
Copy link
Member Author

I confirmed this really makes a difference by doing the following:

Setup a server with a piece of content > 500 MiB. Take zBase for example.
Have a client download this file.

Before this PR, you see the client getting some bytes, till the RCVBUF of the client fills up, and after the SNDBUF of the server fills up .. after that, the connection stalls for a few seconds, while you see the Python process boom in memory footprint. After this you slowly see the data coming in to the client again and slowly you see the memory being deallocated.

What in reality was happening: the server's SNDBUF was full, after which asyncio started to cache all write() to the local task. This can be done on line-speed, without context-switch, causing the SNDBUF to empty in the meantime. When all data is moved from the server to the asyncio write-task, everything starts to move again. When the data is being moved out, memory is being free'd up again.

With this PR, this all changes. If the SNDBUF is full, there is room for 5 more OpenTTD-sized packets. After that, the whole read/write of that client is being put on hold (via an asyncio await). This means this time not all data is being moved to the asyncio-write task, and thus not booming in memory.
When the asyncio-write buffer goes down to 2 OpenTTD-sized packets again, new ones are let in up till 5. Etc etc etc.
In result, the server process never booms in memory. This should prevent the OOMs we now see in production.

Why 2 and 5 you ask? No reason .. experiments showed a low of 0 is wrong, as that can deplete the SNDBUF. A high of more than 5 didn't do anything. So .. empirical evidence: 2 and 5 seem good values for the low and high mark. We might need to tune this, ofc ;)

Copy link
Member

@frosch123 frosch123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\o/ co-routines

@TrueBrain TrueBrain merged commit ba04b7f into OpenTTD:master May 4, 2020
@TrueBrain TrueBrain deleted the performance branch May 4, 2020 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Raise exception if connection is gone
3 participants