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

MetadataReader ReadFile fast path does not work with async file streams #14276

Closed
nguerrera opened this issue Feb 23, 2015 · 6 comments · Fixed by #50367
Closed

MetadataReader ReadFile fast path does not work with async file streams #14276

nguerrera opened this issue Feb 23, 2015 · 6 comments · Fixed by #50367
Assignees
Labels
area-System.Reflection.Metadata bug help wanted [up-for-grabs] Good issue for external contributors needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Milestone

Comments

@nguerrera
Copy link
Contributor

When a FileStream is opened with isAsync: true (.NET Core default!), the ReadFile P/Invoke code that we have in the metadata reader to read small files straight to native memory. ReadFile fails with invalid param error code because our handle was opened for overlapped IO.

For now, I'm changing the code to be more defensive and always fall back to the slow file -> managed heap -> native heap code path. (It currently throws.)

@ericstj: Can you help me figure out how to tweak FileStreamReadLightUp.cs to handle this?

Also, @tmat has already mentioned to me that this code path will slow us down on Linux and Mac. Perhaps, this is the right opportunity to define a portable FileStream.Read overload that reads to unmanaged memory?

@ericstj
Copy link
Member

ericstj commented Feb 23, 2015

If you are getting the handle out of FileStream you should pass all arguments to the FileStream constructor (Either UseAsync or FileOptions).

@nguerrera
Copy link
Contributor Author

@ericstj I don't control the construction of the FileStream. The reader tries to light up and do a fast read via P/Invoke on the Stream that gets passed in already constructed.

@ericstj
Copy link
Member

ericstj commented Feb 23, 2015

In that case you never know if you are getting a sync or overlapped handle. The fact that the default changed is irrelevant since it could have happened before. The way to detect it is to do a ReadFile specifying null for overlapped and see if it fails with ERROR_INVALID_PARAMETER. See VerifyHandleIsSync. You could fallback to actually doing the overlapped IO rather than falling back to managed heap.

@nguerrera nguerrera self-assigned this Feb 24, 2015
@nguerrera
Copy link
Contributor Author

I realize this could have happened before, I just meant that the new default is why it was only uncovered now. I'll look into doing the overlapped IO, but this code is getting gnarly as it has to worry about running on non-Windows.

I'd like to talk some more about having a Stream.Read into byte* with an efficient override in FileStream that just does the right thing. If we think we'll do that, I'd rather wait for that than make this light-up code any more complex.

I'd have to benchmark to see if this is even worth it. We only go down this path for files of size <= 16KB...

@nguerrera
Copy link
Contributor Author

dotnet/corefx#4786 tracks the BCL feature that could make all of this code go away from S.R.Metadata.

@karelz
Copy link
Member

karelz commented Nov 11, 2016

This looks tricky. Where to start is mentioned above.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Jun 29, 2020
@adamsitnik adamsitnik self-assigned this Mar 29, 2021
@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Mar 29, 2021
@adamsitnik adamsitnik modified the milestones: Future, 6.0.0 Mar 29, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 29, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 31, 2021
@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.
Labels
area-System.Reflection.Metadata bug help wanted [up-for-grabs] Good issue for external contributors needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

6 participants