Follow up on Scatter/Gather API changes #58447
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-io Issue DetailsThis PR addresses some feedback that was provided on #57424 after it was merged. cc @stephentoub
|
stephentoub
reviewed
Aug 31, 2021
src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Aug 31, 2021
src/libraries/System.IO.FileSystem/tests/RandomAccess/NoBuffering.Windows.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Aug 31, 2021
| // Using segments with a length of twice the page size adheres to FILE_FLAG_NO_BUFFERING's | ||
| // requirements but not the scatter/gather APIs'. The RandomAccess implementation will | ||
| // see it and issue sequential read/write syscalls per segment, instead of one | ||
| // scatter/gather syscall. This test verifies that fallback behavior. |
There was a problem hiding this comment.
It verifies the fallback behavior when the length requirement isn't met but the others are, yes? What about when, say, the alignment requirement isn't met? Do we already have tests for that elsewhere?
stephentoub
reviewed
Aug 31, 2021
src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
Outdated
Show resolved
Hide resolved
stephentoub
reviewed
Aug 31, 2021
src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
Outdated
Show resolved
Hide resolved
danmoseley
changed the title
adamsitnik
reviewed
Sep 13, 2021
src/libraries/System.Private.CoreLib/src/System/IO/RandomAccess.Windows.cs
Outdated
Show resolved
Hide resolved
adamsitnik
approved these changes
Sep 13, 2021
|
@adamsitnik, @stephentoub, any additional feedback? |
adamsitnik
pushed a commit
that referenced
this pull request
Oct 18, 2021
* Allocate an array of memory handles only if needed. * Remove an unnecessary variable in the multiple-syscall write gather. * Actually verify the content read by the read scatter operation. * Delay allocating native memory. * Verify that the whole file was read in the scatter/gather test. * Test the case when the scatter/gather buffers are acceptable by the Windows API. * Avoid null pointer dereferences when passing an empty segment array. * Test performing scatter/gather I/O with an empty segment array. Co-authored-by: Stephen Toub <[email protected]>
Anipik
pushed a commit
that referenced
this pull request
Oct 19, 2021
…s et. al. (#58423) * Move checking and pinning Windows vectored I/O buffers to a dedicated method. * Refactor the scatter/gather APIs to use the common checking method. And use pinned GCHandles and IntPtrs instead of MemoryHandles when passing the segment array to the bottom-most method. * Shorten the name of the buffer-checking method. * Directly get the pinned array's address instead of calling GCHandle.AddrOfPinnedObject. * Refactor the error handling logic in TryPrepareScatterGatherBuffers. * Allocate the segment array from native memory and at TryPrepareScatterGatherBuffers. * Cache the page size on a static readonly field and add a couple of TODOs. * Make the memory handlers readonly structs. * Add a test. * Reorder some methods with PR feedback taken into consideration. * Stop special-casing scatter/gather operations with zero or one buffer. * Factor the cleaning-up of the segment buffers into a separate method. * Follow up on Scatter/Gather API changes (#58447) * Allocate an array of memory handles only if needed. * Remove an unnecessary variable in the multiple-syscall write gather. * Actually verify the content read by the read scatter operation. * Delay allocating native memory. * Verify that the whole file was read in the scatter/gather test. * Test the case when the scatter/gather buffers are acceptable by the Windows API. * Avoid null pointer dereferences when passing an empty segment array. * Test performing scatter/gather I/O with an empty segment array. Co-authored-by: Stephen Toub <[email protected]> Co-authored-by: Theodore Tsirpanis <[email protected]> Co-authored-by: Theodore Tsirpanis <[email protected]> Co-authored-by: Stephen Toub <[email protected]>
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses some feedback that was provided on #57424 after it was merged.
cc @stephentoub