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

Move FileStream into System.Runtime #18951

Closed
weshaggard opened this issue Oct 13, 2016 · 5 comments
Closed

Move FileStream into System.Runtime #18951

weshaggard opened this issue Oct 13, 2016 · 5 comments
Assignees
Milestone

Comments

@weshaggard
Copy link
Member

Due to various dependencies we need to move FileStream into System.Runtime as well as the implementation into System.Private.CoreLib.

See discussion at dotnet/standard#52

I've started created a proof-of-concept branch that moves just the APIs in the contracts here
weshaggard/corefx@e77e2b3, but the primary work is in the implementation.

cc @ianhays @ericstj @jkotas @stephentoub

@stephentoub
Copy link
Member

as well as the implementation into System.Private.CoreLib

Ouch. There's no way around that?

@weshaggard
Copy link
Member Author

Ouch. There's no way around that?

I'm open to ideas but right now there are reflection APIs that have direct dependencies on FileStream as well as plenty of things in CoreLib that need to read files so no matter what we will have a copy of some amount of it in there.

@jkotas
Copy link
Member

jkotas commented Oct 13, 2016

My comment from other thread:

Pretty much any core runtime implementation I can think of needs to access files. I do not think we are getting a lot of positive value by keeping it up stack (except for WinRT - but that can be dealt with in isolation) - if the FileStream is upstack, the core runtime needs to have its own LowLevelFileStream sort of type anyway.

Some thoughts on implementation:

  • I think we may want to keep using the corefx System.Native PAL even in CoreLib implementation. It is a cycle/private link between the repos but we have number of these anyway, so it does not sound like a big deal and it should be a net win for engineering efficiency.
  • The WinRT parts will need to be done via upstack callback (or other special plumbing), similar to how other WinRT parts are done (e.g. the WinRT resource manager).

@stephentoub
Copy link
Member

Pretty much any core runtime implementation I can think of needs to access files

Of course. And we have the FileStream in coreclr that we use for that purpose, so certainly it'd be nice to consolidate down to a single one. A few concerns:

  • The native dependencies, which you mention, and I agree it likely makes sense to think of the repo boundary as not being there and just use System.Native. I do not expect enough churn in the calls made between FileStream and the shim to be concerned about it.
  • Dependencies on other components in corefx. For example, FileStream.CopyToAsync now has a variety of optimizations that make it much more efficient than just using a read/write copy loop, and as part of this it uses ArrayPool. This allowed ASP.NET to switch to using FileStream's CopyToAsync rather than their own read/write loop that uses ArrayPool. When the implementation moves into corelib, we'll either need to ditch the pooling (which would force ASP.NET to move off of it), move ArrayPool into corelib, or come up with another pooling mechanism for use within corelib. There are benefits to being able to use pooling in corelib, as there are then other places we could utlize it (e.g. the base Stream.CopyToAsync), but it's something to think through carefully.

@jkotas
Copy link
Member

jkotas commented Oct 14, 2016

move ArrayPool into corelib

This makes sense to me. The .NET Core stack is likely going to be using ArrayPool more heavily - having it in corelib makes it easier to have private interface with the GC and optimize the pooling strategy.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants