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

ensure MetadataReader fast path works with every FileStream on every OS #50367

Merged
merged 4 commits into from
Mar 31, 2021

Conversation

adamsitnik
Copy link
Member

This code predates me, but according to my understanding:

  1. We had a Windows-specific optimization:

private static bool IsReadFileAvailable =>
#if NETCOREAPP
OperatingSystem.IsWindows();
#else
Path.DirectorySeparatorChar == '\\';
#endif

  1. That was calling ReadFile syscall:

int result = Interop.Kernel32.ReadFile(handle, buffer, size, out int bytesRead, IntPtr.Zero);

  1. That was working only for sync FileStreams:

// We used to throw here, but this is where we land if the FileStream was
// opened with useAsync: true, which is currently the default on .NET Core.
// https://github.com/dotnet/corefx/pull/987 filed to look in to how best to
// handle this, but in the meantime, we'll fall back to the slower code path
// just as in the case where the native API is unavailable in the current platform.

Based on the comment from #14276

Perhaps, this is the right opportunity to define a portable FileStream.Read overload that reads to unmanaged memory?

It seems that this code was created before we have added a Stream.Read(Span) overload that allows for reading from a file to an unmanaged memory buffer.

Moreover, I think that partial reads were not handled properly, as in cases when bytesRead != requestedSize the block.Pointer was not incremented properly:

if (!isFileStream || !FileStreamReadLightUp.TryReadFile(stream, block.Pointer, start, size))
{
stream.CopyTo(block.Pointer, size);

fixes #14276

@adamsitnik adamsitnik requested a review from krwq March 30, 2021 08:26
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me as is.

@stephentoub
Copy link
Member

Consider changing the signature of TryReadFile from:

bool TryReadFile(Stream stream, byte* buffer, int size, out int bytesRead)

to instead be:

int ReadFile(Stream stream, byte* buffer, int size)

The sole caller of TryReadFile always proceeds to use the stream and the out bytesRead if TryReadFile returns false, so it seems like it'd be more understandable if instead of:

int bytesRead = 0;
if (!isFileStream || !FileStreamReadLightUp.TryReadFile(stream, block.Pointer, size, out bytesRead))
{
    stream.CopyTo(block.Pointer + bytesRead, size - bytesRead);

the caller was:

int bytesRead = 0;
if (!isFileStream ||
    (bytesRead = FileStreamReadLightUp.ReadFile(stream, block.Pointer, size)) != size)
{
    stream.CopyTo(block.Pointer + bytesRead, size - bytesRead);

That way it's clear to the caller that the stream may have still advanced even if TryReadFile returned false, that bytesRead is still meaningful, etc., and it's clear why bytesRead is being factored into the CopyTo.

@adamsitnik adamsitnik merged commit bce310d into dotnet:main Mar 31, 2021
@adamsitnik adamsitnik deleted the MetadataReaderReadFile branch March 31, 2021 06:53
@ghost ghost locked as resolved and limited conversation to collaborators Apr 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MetadataReader ReadFile fast path does not work with async file streams
4 participants