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

[BUG] Saltstack uses ancient Tornado with inefficient buffer management #62960

Closed
szjur opened this issue Oct 26, 2022 · 2 comments
Closed

[BUG] Saltstack uses ancient Tornado with inefficient buffer management #62960

szjur opened this issue Oct 26, 2022 · 2 comments
Labels
Bug broken, incorrect, or confusing behavior dependency underlying Salt dependency issue Deprecation needs-triage

Comments

@szjur
Copy link

szjur commented Oct 26, 2022

Description
Is there any specific reason Saltstack clings onto ancient Tornado with inefficient buffer management? In 2017 Tornado got improvements which eliminated costly memory copy operations:

tornadoweb/tornado#2147
tornadoweb/tornado#2169

While analysing memory leaks in Salt master with tracemalloc salt/ext/tornado/iostream.py was identified as the main memory hog. Indeed planting some debugs there I could see that _write_buffer could easily reach hundreds of megabytes under high load of large events. The legacy tornado logic for shrinking the buffer is not too aggressive, which directly contributes to high memory consumption:

https://github.com/saltstack/salt/blob/master/salt/ext/tornado/iostream.py#L872

if self._write_buffer_pos > self._write_buffer_size:

If my logic is correct, this will only shrink the buffer if it's at least half empty.

Setup
Irrelevant - applies to latest code from Saltstack repo.

Steps to Reproduce the behavior
Impose very high load of big events on Salt and see poor iostream performance.

Expected behavior
A clear and concise description of what you expected to happen.

Screenshots
If applicable, add screenshots to help explain your problem.

Versions Report
Any

@szjur szjur added Bug broken, incorrect, or confusing behavior needs-triage labels Oct 26, 2022
@szjur szjur changed the title [BUG] Saltstack uses ancient Tornado with ineffecient buffer management [BUG] Saltstack uses ancient Tornado with inefficient buffer management Oct 26, 2022
@OrangeDog
Copy link
Contributor

OrangeDog commented Oct 26, 2022

The developers have been trying for years to upgrade Tornado (e.g. #56169), and have never managed to finish the work.

The current plan appears to be to drop Tornado entirely: #61380

@szjur
Copy link
Author

szjur commented Oct 26, 2022

Asyncio seems reasonable, fingers crossed :-) That Tornado improvement involving _StreamBuffer class was only introduced in 5.0. I probably won't dare backporting it but I'll probably have a go at tweaking the condition to release the unused parts of _write_buffer sooner.

@OrangeDog OrangeDog added dependency underlying Salt dependency issue Deprecation labels Nov 17, 2022
@OrangeDog OrangeDog added this to the Chlorine v3007.0 milestone Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior dependency underlying Salt dependency issue Deprecation needs-triage
Projects
None yet
Development

No branches or pull requests

2 participants