More WriteGather fixes #109826
More WriteGather fixes #109826
Conversation
adamsitnik
commented
Nov 14, 2024
•
adamsitnik
commented
- don't run these tests in parallel, as each test cases uses more than 4 GB ram and disk
- fix the test: handle incomplete reads that should happen when we hit the max buffer limit
- incomplete write fix:
- pin the buffers only once
- when re-trying, do that only for the actual reminder
- handle Int32 overflow on macOS
…4 GB ram and disk!
…the max buffer limit
- pin the buffers only once - when re-trying, do that only for the actual reminder
|
Tagging subscribers to this area: @dotnet/area-system-io |
| ReadOnlyMemory<byte> buffer = buffers[i]; | ||
| totalBytesToWrite += buffer.Length; | ||
|
|
||
| MemoryHandle memoryHandle = buffer.Pin(); |
There was a problem hiding this comment.
So far, for incomplete writes we were pinning the memory for every retry attempt. I am not sure if this can create some kind of edge case bugs, but I think we can do it just once, before we enter the main loop
| { | ||
| if (asyncMethod) | ||
| { | ||
| await RandomAccess.WriteAsync(sfh, writeBuffers, fileOffset); | ||
| bytesRead = await RandomAccess.ReadAsync(sfh, readBuffers, fileOffset); |
There was a problem hiding this comment.
this was a test bug, the test assumed that the read won't ever be a partial read
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Co-authored-by: Michał Petryka <35800402+MichalPetryka@users.noreply.github.com>
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…o avoid the process getting killed on Linux when OOM happens)
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…th of all vectors overflows a 32-bit integer.
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| { | ||
| break; | ||
| } | ||
| MemoryHandle memoryHandle = buffer.Pin(); |
There was a problem hiding this comment.
What happens if Pin throws an exception (or if the IReadOnlyList indexer does)? Won't we now leak all the previously created handles?
There was a problem hiding this comment.
@stephentoub good catch, I forgot that the users can provide a custom MemoryManager and have Pin throw.
| allowedCount = IOV_MAX; | ||
| totalLength += vectors[i].Count; | ||
|
|
||
| if (totalLength > INT_MAX && i > 0) |
There was a problem hiding this comment.
Why is the && i > 0 necessary? Can `vectors[i].Count be greater than INT_MAX in practice?
There was a problem hiding this comment.
Why is the
&& i > 0necessary?
To avoid a situation where we pass 0 as iovcnt to the sys-call, it succeeds and the managed layer decides to continue the writes and ends up in an endless loop.
Can `vectors[i].Count be greater than INT_MAX in practice?
Not right now, as Memory.Length is an Int32 as of today.
@stephentoub is it worth to be so paranoid and defensive in your opinion?
There was a problem hiding this comment.
Why is the
&& i > 0necessary?To avoid a situation where we pass
0asiovcntto the sys-call, it succeeds and the managed layer decides to continue the writes and ends up in an endless loop.
I'm not understanding. I thought this && i > 0 check was here to skip the first entry, but because Count can never be greater than INT_MAX, the totalLength > INT_MAX clause will never be true when i == 0.
I'm not clear on what this condition has to do with 0. If vectorCount is 0, we'll not be in this loop at all. And if Count is 0, either the totalLength would have already exceeded INT_MAX and would have been flagged by a previous iteration, or totalLength is not greater than INT_MAX.
I'm missing something. What? :)
There was a problem hiding this comment.
I'm missing something.
I expect that Count will be able to be more than Int32 one day. But most likely I am overthinking here.
There was a problem hiding this comment.
I expect that Count will be able to be more than Int32 one day.
Ok. It'd be worth a comment, I think.
There was a problem hiding this comment.
I added an assert and removed the condition.
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…han Int32.MaxValue
|
/azp run runtime-libraries-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
