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

Use a wrapper with unbuffered I/O for farming on Windows #2581

Merged
merged 10 commits into from
Mar 6, 2024

Conversation

nazar-pc
Copy link
Member

@nazar-pc nazar-pc commented Mar 3, 2024

This PR makes farming on Windows use unbuffered I/O, which avoids memory usage issues, but also has annoying alignment requirements. Users are testing this on the forum already.

Since reads we're doing during farming are tiny, I didn't implement fast path for cases where both read target and memory buffer into which read bytes are written are already aligned (it will not be a typical case for farming anyway), but that could be a hypothetical improvement.

Farming is a fairly limited in scope code path, so it was relatively easy to add there. We do have other places where reads are doing the old way though and I think there will be value of expanding there as well (left TODO about this). We can replace the whole File with this UnbufferedIoFileWindows for both reads and writes a bit later by implementing FileExt on it, but this PR should already be a huge improvement as is.

PR includes a few basic tests with aligned and unaligned reads that are cross-platform for convenience, but UnbufferedIoFileWindows only makes sense to use on Windows due to unnecessary copying on other platforms that do not suffer from memory usage bugs in the first place.

I also noticed a bit better auditing and proving performance with these changes (nothing to phone home about, but still), which are not very surprising.

Code contributor checklist:

@nazar-pc
Copy link
Member Author

nazar-pc commented Mar 3, 2024

Not great feedback here on farming success, so converting to draft for now

@nazar-pc nazar-pc marked this pull request as draft March 3, 2024 19:11
Base automatically changed from farmer-caching-tweaks to main March 4, 2024 09:44
@nazar-pc
Copy link
Member Author

nazar-pc commented Mar 5, 2024

Added workarounds for more Windows bugs, specifically al8n/fs4-rs#13 (https://learn.microsoft.com/en-us/answers/questions/1608540/getfileinformationbyhandle-followed-by-read-with-f), should work this time 🤞

@nazar-pc nazar-pc marked this pull request as ready for review March 6, 2024 05:22
@nazar-pc
Copy link
Member Author

nazar-pc commented Mar 6, 2024

Farmers confirm latest revision I just pushed finally seems to work, even though it is a pile of Windows-specific hacks.

@nazar-pc
Copy link
Member Author

nazar-pc commented Mar 6, 2024

Next steps:

  • use new wrapper for all reads
  • likely expose reading of the whole sector as a CLI option

@nazar-pc nazar-pc added this pull request to the merge queue Mar 6, 2024
Merged via the queue into main with commit 446ecca Mar 6, 2024
9 checks passed
@nazar-pc nazar-pc deleted the unbuffered-farming-io-windows branch March 6, 2024 08:04
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.

2 participants