Added a factory to FileStreamCompletionSource #16190

davidfowl · 2018-02-04T00:50:31Z

Added a factory method to FileStreamCompletionSource that
picks the an impl or FileStreamCompletionSource or MemoryFileStreamCompletionSource based on the buffer matching the underlying stream's internal buffer.
Delegate to memory.Retain if the underlying ReadOnlyMemory is backed by an array that isn't the stream's internal buffer.

PS: I haven't run any tests locally yet 😄. I will when I find out how to do so.

Addresses https://github.com/dotnet/corefx/issues/26817

- Change FileStreamCompletionSource to take a ReadOnlyMemory<byte> instead of byte[] - Delegate to memory.Retain if the underlying ReadOnlyMemory<byte> is backed by an array that isn't the stream's internal buffer.

benaadams · 2018-02-04T01:01:25Z

I haven't run any tests locally yet

Can run the corefx CI ones; assuming coreclr and corefx are in sync with their apis

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

stephentoub · 2018-02-04T12:13:50Z

src/mscorlib/shared/System/IO/FileStreamCompletionSource.Win32.cs

+                    // If the memory passed in isn't the stream's internal buffer, then delegate to
+                    // memory.Retain instead of passing the buffer to AllocateNativeOverlapped. In 
+                    // some cases, this will result in less pinning (in case the underlying memory is backed)
+                    // by pre-pinned buffers


Nit: closing paren should be at the end of the sentence rather than after "backed"

benaadams · 2018-02-04T12:20:28Z

coreclr and corefx out of sync

MembersMustExist : Member 
'System.Runtime.Intrinsics.X86.Sse2.ConvertScalarToVector128UInt32Scalar(System.UInt32)'
does not exist in the implementation but it does exist in the contract.

/cc @tannergooding

stephentoub · 2018-02-04T13:35:41Z

src/mscorlib/shared/System/IO/FileStreamCompletionSource.Win32.cs

            private NativeOverlapped* _overlapped; // Overlapped class responsible for operations in progress when an appdomain unload occurs
            private long _result; // Using long since this needs to be used in Interlocked APIs

+            private MemoryHandle _handle; // mutable struct; do not make this readonly


I'd have to measure again, but just looking at the existing fields, looks like this will increase the size of the object by app 33%. For the common case of the internal buffer being used, that's unfortunate. Can we avoid that? That was one of the reasons for the separate derived type. How about just creating a factory method that contains the below logic and then use that at the two call sites rather than the current naive logic based purely on whether an array is wrapped?

You think the one extra field is worth that complexity?

It's not one field. It's three ptr-size fields.

Is this created for every Read, could it be reused across Reads attached to the Stream?

Every non-buffered read and write on an async FileStream.

Sure, when you originally did this was it pre-emptive? What did you measure that made you split the classes? Was it just object size?

You could do something similar with the cancellation registration and remove the _cancellationRegistration field if the CanBeCancelled is false.

What did you measure that made you split the classes? Was it just object size?

Yes, it avoided regressing bytes allocated from previous releases.

You could do something similar with the cancellation registration and remove the _cancellationRegistration field if the CanBeCancelled is false.

Yes, we could, but not without non-trivial extra complexity: it's accessed from the callback, not just Dispose. Also, MemoryHandle is bigger, and from a complexity perspective it's not worth classes for all combinations. I do think it's work two classes, that we already have.

I'll send an update

- Added a factory method to FileStreamCompletionSource that picks the an impl or FileStreamCompletionSource or MemoryFileStreamCompletionSource based on the buffer matching the underlying stream's internal buffer.

davidfowl · 2018-02-04T14:16:00Z

src/mscorlib/shared/System/IO/FileStreamCompletionSource.Win32.cs


            // Using RunContinuationsAsynchronously for compat reasons (old API used Task.Factory.StartNew for continuations)
-            internal FileStreamCompletionSource(FileStream stream, int numBufferedBytes, byte[] bytes)
+            protected FileStreamCompletionSource(FileStream stream, int numBufferedBytes, byte[] bytes)


Made this protected so that it can't be newed up. Forces callers to find the factory.

Sounds good

davidfowl · 2018-02-04T14:20:07Z

🆙 📅 (updated)

stephentoub · 2018-02-04T15:38:06Z

src/mscorlib/shared/System/IO/FileStreamCompletionSource.Win32.cs

+                    }
+                }
+
+                return new MemoryFileStreamCompletionSource(stream, numBufferedBytesRead, memory);


Nit: It's ok if you prefer it this way; if it were me, I'd have written it at least with the condition inverted, and maybe condensed to more like:

// If the memory passed in is the stream's internal buffer, we can use the base FileStreamCompletionSource, // which has a PreAllocatedOverlapped with the memory already pinned. Otherwise, we use the derived // MemoryFileStreamCompletionSource, which Retains the memory, which will result in less pinning in the case // where the underlying memory is backed by pre-pinned buffers. return MemoryMarshal.TryGetArray(memory, out ArraySegment<byte> arraySegment) && ReferenceEquals(arraySegment.Array, stream._buffer) ? new FileStreamCompletionSource(stream, numBufferedBytesRead, arraySegment.Array) : new MemoryFileStreamCompletionSource(stream, numBufferedBytesRead, memory);

This is clearer.

stephentoub · 2018-02-04T15:39:30Z

src/mscorlib/shared/System/IO/FileStreamCompletionSource.Win32.cs

+                    // memory.Retain instead of passing the buffer to AllocateNativeOverlapped. In 
+                    // some cases, this will result in less pinning (in case the underlying memory is backed
+                    // by pre-pinned buffers)
+                    if (!ReferenceEquals(buffer, stream._buffer))


We can remove the condition in the base class now, right? i.e. the code that's currently:

_overlapped = (bytes == null || ReferenceEquals(bytes, _stream._buffer)) && _stream.CompareExchangeCurrentOverlappedOwner(this, null) == null ? _stream._fileHandle.ThreadPoolBinding.AllocateNativeOverlapped(_stream._preallocatedOverlapped) : _stream._fileHandle.ThreadPoolBinding.AllocateNativeOverlapped(s_ioCallback, this, bytes);

could instead be:

Debug.Assert(bytes == null || ReferenceEquals(bytes, _stream._buffer)); _overlapped = _stream.CompareExchangeCurrentOverlappedOwner(this, null) == null ? _stream._fileHandle.ThreadPoolBinding.AllocateNativeOverlapped(_stream._preallocatedOverlapped) : _stream._fileHandle.ThreadPoolBinding.AllocateNativeOverlapped(s_ioCallback, this, null);

?

Yep., that's right. I'll tweak it. Actually, we don't need to pass the byte[] to FileStreamCompletionSource at all, it's only being used to asset that it matches the internal stream buffer.

I'll leave it for the assert.

stephentoub

Thanks for fixing this up, David. A couple remaining comments, otherwise LGTM.

- Cleanup the condition - Flip to Debug.Assert in FileStreamCompletionSource

davidfowl · 2018-02-04T15:56:16Z

OK I think it's done forreal this time 😄

stephentoub

😄

tannergooding · 2018-02-04T16:26:38Z

coreclr and corefx out of sync

@benaadams, CoreCLR has the change (#16147) and CoreFX has the change consuming that (dotnet/corefx#26776), but it looks like CoreCLR does not have the updated CoreFX bits that contain yet 😄

stephentoub · 2018-02-04T16:42:02Z

I thought these legs pulled/built live source from the corefx repo. Is that not the case?

tannergooding · 2018-02-04T16:45:28Z

I'm not sure, but given the changes merged and the error message Ben linked, I would guess not.

benaadams · 2018-02-04T16:50:07Z

I think it pulls version referenced by the build tools update PRs that dotnetbot makes

davidfowl · 2018-02-04T17:01:42Z

I'm assuming somebody will just merge this at some point 😄

benaadams · 2018-02-04T17:07:54Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

benaadams · 2018-02-04T17:10:18Z

Not sure what this one is as it just seems to be updating coreclr #16191

Don't see any corefx update PRs

benaadams · 2018-02-04T17:47:04Z

MembersMustExist : Member 
'System.Runtime.Intrinsics.X86.Sse2.ConvertScalarToVector128UInt32Scalar(System.UInt32)' 
does not exist in the implementation but it does exist in the contract.

benaadams · 2018-02-04T18:40:20Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

jkotas · 2018-02-04T19:11:46Z

I thought these legs pulled/built live source from the corefx repo. Is that not the case?

It used to be like that, but it was changed recently. I have submitted #16199 to revert it since it make things clearly worse.

benaadams · 2018-02-04T19:39:46Z

Is a change, but still out of sync?

error CS0433: The type 'Vector' exists in both 'System.Numerics.Vectors, Version=4.1.4.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' 
and 'System.Private.CoreLib, Version=4.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e'

jkotas · 2018-02-04T19:50:59Z

Is a change, but still out of sync?

It needs Update CoreFX PR to get it in sync. The last Update CoreFX was #16180 (2 days ago).

jkotas · 2018-02-04T19:58:19Z

I am going to merge #16199 as soon as it as gets green to get this unstuck.

davidfowl · 2018-02-04T20:04:13Z

Is my PR being held hostage? 😃

stephentoub · 2018-02-04T20:05:18Z

If we can't get the automated CI tests to work, someone needs to run the tests locally before merging. If you're not set up to do so, I can, it'll just be a little while, and Jan's PR fixing CI might beat me to it.

stephentoub · 2018-02-04T20:59:23Z

I pulled down the PR and ran the tests locally. There are 37 failing tests out of 6140, but they're all the same root cause:

System.ArgumentNullException : Value cannot be null.
Parameter name: preAllocated
Stack Trace:
       d:\repos\coreclr\src\mscorlib\src\System\Threading\ClrThreadPoolBoundHandle.cs(175,0): at System.Threading.ThreadPoolBoundHandle.AllocateNativeOverlapped(PreAllocatedOverlapped preAllocated)
       d:\repos\coreclr\src\mscorlib\shared\System\IO\FileStreamCompletionSource.Win32.cs(52,0): at System.IO.FileStream.FileStreamCompletionSource..ctor(FileStream stream, Int32 numBufferedBytes, Byte[] bytes)
       d:\repos\coreclr\src\mscorlib\shared\System\IO\FileStreamCompletionSource.Win32.cs(228,0): at System.IO.FileStream.FileStreamCompletionSource.Create(FileStream stream, Int32 numBufferedBytesRead, ReadOnlyMemory`1 memory)
    ...

stephentoub · 2018-02-04T21:13:04Z

My rough guess:
I probably didn't previously add tests for the non-array memory path combined with not having an internal buffer. This change uses that path even when the memory does wrap an array. In other words, I think this is highlighting a latent bug that'll just be hit much more often now.

I expect the fix is to just make sure we've initialized the preallocated overlapped. I'll take a look tonight or in the morning.

davidfowl · 2018-02-04T21:19:02Z

@stephentoub thanks for running the tests! I'll take a look as well.

stephentoub · 2018-02-05T15:00:40Z

@davidfowl, the simple fix you can make in this PR is to just add bytes != null && as the first condition in this line:
https://github.com/dotnet/coreclr/pull/16190/files#diff-742d93872313d38656fb88ad801f4d7bR52
i.e.

_overlapped = bytes != null && _stream.CompareExchangeCurrentOverlappedOwner(this, null) == null ?

With that all of the tests pass.

The issue here is that the _preallocatedOverlapped is tied to _buffer: _preallocatedOverlapped is only initialized when _buffer is initialized, because if the _preallocatedOverlapped exists, we trust that it pins _buffer. The tests that failed didn't yet have the buffer because all reads/writes thus far were bigger than the buffer size and thus skipped it.

The fix I suggested above leaves things in a state no worse than today, where we only use the _preallocatedOverlapped for operations targeting the internal _buffer. However, it would be nice to do better than that. Using the _preallocatedOverlapped is more efficient than not, and so it'd be nice to be able to take that path even when targeting a Memory that's not _buffer, as in the ASP.NET case where I believe it frequently creates FileStreams with a bufferSize of 1 in order to avoid the buffer ever being used. I see a few options there:

Do nothing beyond the above fix.
In the factory method, invoke GetBuffer. This will force the buffer into existence if it isn't already. This is fine if we just happen to be doing a particularly large read or write and expect subsequent reads/writes to target the buffer, but it would result in an unnecessary buffer allocation if all of the reads/writes on the stream are going to be larger than the buffer.
Special-case _bufferSize == 1, in which case we can simply change the GetBuffer logic to special-case _bufferSize == 1 to use Array.Empty, and then invoke GetBuffer in the FileStreamCompletionSource factory method to ensure both the buffer and the overlapped have been created.
Have two PreallocatedOverlappeds, one for the internal buffer and one with a pinData of null. This is the most general solution, but also requires more bookkeeping.

(2) doesn't seem very good. Might be worth experimenting with (4) to see what kind of complexity it adds and whether it's worth it.

None of that should hold up this PR, though; we can just add that condition and consider this one done.

benaadams · 2018-02-05T18:11:01Z

@dotnet-bot test Windows_NT x64 Checked corefx_baseline
@dotnet-bot test Ubuntu x64 Checked corefx_baseline

stephentoub · 2018-02-05T20:51:58Z

The failures are known runtime bugs:
https://github.com/dotnet/coreclr/issues/16201
https://github.com/dotnet/coreclr/issues/16202

* Merged FileStreamCompletionSource and MemoryCompletionSource - Change FileStreamCompletionSource to take a ReadOnlyMemory<byte> instead of byte[] - Delegate to memory.Retain if the underlying ReadOnlyMemory<byte> is backed by an array that isn't the stream's internal buffer. * Brought back MemoryFileStreamCompletionSource - Added a factory method to FileStreamCompletionSource that picks the an impl or FileStreamCompletionSource or MemoryFileStreamCompletionSource based on the buffer matching the underlying stream's internal buffer. * PR feedback - Cleanup the condition - Flip to Debug.Assert in FileStreamCompletionSource * Check for null before using the PreAllocatedOverlapped Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>

* Merged FileStreamCompletionSource and MemoryCompletionSource - Change FileStreamCompletionSource to take a ReadOnlyMemory<byte> instead of byte[] - Delegate to memory.Retain if the underlying ReadOnlyMemory<byte> is backed by an array that isn't the stream's internal buffer. * Brought back MemoryFileStreamCompletionSource - Added a factory method to FileStreamCompletionSource that picks the an impl or FileStreamCompletionSource or MemoryFileStreamCompletionSource based on the buffer matching the underlying stream's internal buffer. * PR feedback - Cleanup the condition - Flip to Debug.Assert in FileStreamCompletionSource * Check for null before using the PreAllocatedOverlapped Signed-off-by: dotnet-bot <[email protected]>

* Merged FileStreamCompletionSource and MemoryCompletionSource - Change FileStreamCompletionSource to take a ReadOnlyMemory<byte> instead of byte[] - Delegate to memory.Retain if the underlying ReadOnlyMemory<byte> is backed by an array that isn't the stream's internal buffer. * Brought back MemoryFileStreamCompletionSource - Added a factory method to FileStreamCompletionSource that picks the an impl or FileStreamCompletionSource or MemoryFileStreamCompletionSource based on the buffer matching the underlying stream's internal buffer. * PR feedback - Cleanup the condition - Flip to Debug.Assert in FileStreamCompletionSource * Check for null before using the PreAllocatedOverlapped Commit migrated from dotnet/coreclr@39914cd

Merged FileStreamCompletionSource and MemoryCompletionSource

347acf0

- Change FileStreamCompletionSource to take a ReadOnlyMemory<byte> instead of byte[] - Delegate to memory.Retain if the underlying ReadOnlyMemory<byte> is backed by an array that isn't the stream's internal buffer.

stephentoub reviewed Feb 4, 2018

View reviewed changes

Brought back MemoryFileStreamCompletionSource

d16981f

- Added a factory method to FileStreamCompletionSource that picks the an impl or FileStreamCompletionSource or MemoryFileStreamCompletionSource based on the buffer matching the underlying stream's internal buffer.

davidfowl commented Feb 4, 2018

View reviewed changes

davidfowl changed the title ~~Merged FileStreamCompletionSource and MemoryCompletionSource~~ Added a factory to FileStreamCompletionSource Feb 4, 2018

stephentoub reviewed Feb 4, 2018

View reviewed changes

stephentoub approved these changes Feb 4, 2018

View reviewed changes

PR feedback

8bdba65

- Cleanup the condition - Flip to Debug.Assert in FileStreamCompletionSource

stephentoub approved these changes Feb 4, 2018

View reviewed changes

jkotas mentioned this pull request Feb 4, 2018

Revert "For corefx testing, use a matching corefx repo version git hash" #16199

Merged

Check for null before using the PreAllocatedOverlapped

e5cd2ad

stephentoub merged commit 39914cd into dotnet:master Feb 5, 2018

stephentoub mentioned this pull request Feb 7, 2018

Add FileStream tests for custom OwnedMemory with Read/WriteAsync dotnet/corefx#26931

Merged

JeremyKuhne added the area-System.IO label Feb 18, 2018

JeremyKuhne added this to the 2.1.0 milestone Feb 18, 2018

davidfowl mentioned this pull request Jan 31, 2020

Use PreallocatedOverlapped when internal FileStream buffer isn't being used on Windows dotnet/runtime#25074

Closed

stephentoub mentioned this pull request Mar 9, 2021

FileStream rewrite part II dotnet/runtime#48813

Merged

Added a factory to FileStreamCompletionSource #16190

Added a factory to FileStreamCompletionSource #16190

Uh oh!

Conversation

davidfowl commented Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Feb 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Feb 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Feb 4, 2018

Uh oh!

stephentoub Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Feb 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Feb 4, 2018

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

tannergooding commented Feb 4, 2018

Uh oh!

stephentoub commented Feb 4, 2018

Uh oh!

tannergooding commented Feb 4, 2018

Uh oh!

benaadams commented Feb 4, 2018

Uh oh!

davidfowl commented Feb 4, 2018

Uh oh!

benaadams commented Feb 4, 2018

Uh oh!

benaadams commented Feb 4, 2018

Uh oh!

benaadams commented Feb 4, 2018

Uh oh!

benaadams commented Feb 4, 2018

Uh oh!

jkotas commented Feb 4, 2018

Uh oh!

benaadams commented Feb 4, 2018

davidfowl commented Feb 4, 2018 •

edited

Loading

stephentoub Feb 4, 2018 •

edited

Loading

stephentoub Feb 4, 2018 •

edited

Loading

stephentoub commented Feb 4, 2018 •

edited

Loading

stephentoub commented Feb 4, 2018 •

edited

Loading