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

Increase API buffer cutoff to 128KiB #18286

Closed
carlos-zamora opened this issue Dec 4, 2024 · 0 comments · Fixed by #18287
Closed

Increase API buffer cutoff to 128KiB #18286

carlos-zamora opened this issue Dec 4, 2024 · 0 comments · Fixed by #18287
Labels
In-PR This issue has a related PR Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting

Comments

@carlos-zamora
Copy link
Member

I figured out why this happens: When we read from the console pipe, we currently need to provide a buffer where the message is copied into, similar to ReadFile. In order to slightly amortize the cost of allocating a buffer we reuse the previous allocation unless the previous one was >16KiB and the next one is less than half the size. This avoids issues where a huge 100MB buffer never gets freed.
This however does not work well for this usage of the API, because the GetConsoleProcessList call needs 128KiB and the PeekConsoleInput call only needs 20 (?) bytes. This causes the buffer to be repeatedly freed and reallocated.

IMO there's 3 things we should do:

  • cygwin should avoid making "large" data requests if there's no reason for that. That DWORD buf[65536]; buffer is 128KiB large and in practice maybe 16 byte of that will ever be used. It could improve the code by only increasing the request size if the return value is equal to the buffer size.
  • We should increase the cutoff size from 16KiB to 128KiB, because that's the size of the cat chunk size anyway.
  • We should switch to a cheap allocator for processing console messages in the future, because malloc is a poor fit for it. But this doesn't fix the issue for large allocations, since we still need to be conservative about our resident set size. This means that even with a custom allocator, cygwin's usage pattern may still be suboptimal.

Originally posted by @lhecker in #18264

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Dec 4, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Dec 4, 2024
DHowett pushed a commit that referenced this issue Dec 5, 2024
This increases the console IO buffer size to retain at least 128KiB as
this matches the default buffer size of `cat`. This avoids allocator
churn due to constantly freeing and reallocating buffers. In the future
this should ideally use a better suited, cheap allocator.

Closes #18286
DHowett pushed a commit that referenced this issue Jan 8, 2025
This increases the console IO buffer size to retain at least 128KiB as
this matches the default buffer size of `cat`. This avoids allocator
churn due to constantly freeing and reallocating buffers. In the future
this should ideally use a better suited, cheap allocator.

Closes #18286

(cherry picked from commit aa5459d)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgWLtjY
Service-Version: 1.22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In-PR This issue has a related PR Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant