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

[Proposal] Passing custom buffer to FileStream #52321

Closed
sakno opened this issue May 5, 2021 · 12 comments
Closed

[Proposal] Passing custom buffer to FileStream #52321

sakno opened this issue May 5, 2021 · 12 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO untriaged New issue has not been triaged by the area owner

Comments

@sakno
Copy link
Contributor

sakno commented May 5, 2021

Background and Motivation

FileStream has its own buffer for I/O. But it cannot be passed from outside. If the application requires a lot of FileStream objects with short lifetime there is not way to reuse the buffer between them. For instance, logging to files.Rotation of log files leads to dispose of old streams and create new ones. Also, it's reasonable to use memory pool (e.g. ArrayPool<T> or MemoryPool<T>) as a source of such buffers.

Proposed API

public class FileStream
{
  public FileStream(string path, FileMode mode, FileAccess access, FileShare share, Memory<byte> buffer, FileOptions options)
}

Usage Examples

var buffer = ArrayPool<byte>.Shared.Rent(4096);
using var fs = new FileStream("dummy.txt", FileMode.OpenOrCreate, FileAccess.Wrtie, FileShare.None, buffer.AsMemory(), FileOptions.None);

Alternative Designs

To mitigate risk of sharing the same buffer between different instances of file stream we could offer passing of memory pool:

public class FileStream
{
  public FileStream(string path, FileMode mode, FileAccess access, FileShare share, ArrayPool<byte> buffer, FileOptions options);
  public FileStream(string path, FileMode mode, FileAccess access, FileShare share, MemoryPool<byte> buffer, FileOptions options);
}

Risks

  • The same buffer can be passed to two or more streams
  • The buffer has been returned to the pool while FileStream still in use
@sakno sakno added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 5, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.IO untriaged New issue has not been triaged by the area owner labels May 5, 2021
@ghost
Copy link

ghost commented May 5, 2021

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

FileStream has its own buffer for I/O. But it cannot be passed from outside. If the application requires a lot of FileStream objects with short lifetime there is not way to reuse the buffer between them. For instance, logging to files.Rotation of log files leads to dispose of old streams and create new ones. Also, it's reasonable to use memory pool (e.g. ArrayPool<T> or MemoryPool<T>) as a source of such buffers.

Proposed API

public class FileStream
{
  public FileStream(string path, FileMode mode, FileAccess access, FileShare share, Memory<byte> buffer, FileOptions options)
}

Usage Examples

var buffer = ArrayPool<byte>.Shared.Rent(4096);
using var fs = new FileStream("dummy.txt", FileMode.OpenOrCreate, FileAccess.Wrtie, FileShare.None, buffer.AsMemory(), FileOptions.None);

Alternative Designs

To mitigate risk of sharing the same buffer between different instances of file stream we could offer passing of memory pool:

public class FileStream
{
  public FileStream(string path, FileMode mode, FileAccess access, FileShare share, ArrayPool<byte> buffer, FileOptions options);
  public FileStream(string path, FileMode mode, FileAccess access, FileShare share, MemoryPool<byte> buffer, FileOptions options);
}

Risks

  • The same buffer can be passed to two or more streams
  • The buffer has been returned to the pool while FileStream still in use
Author: sakno
Assignees: -
Labels:

api-suggestion, area-System.IO, untriaged

Milestone: -

@davidfowl
Copy link
Member

Pass a buffer size of 1 a d you'll disable the FileStream's internal buffer. This shouldn't be needed

@sakno
Copy link
Contributor Author

sakno commented May 6, 2021

Not sure that this works on Linux/MacOS/FreeBSD. In .NET 6, Net5CompatFileStreamStrategy is used for Unix-like platforms. For Windows, there is a condition which analyzes value 1. Moreover, I don't want to disable internal buffer and move the buffer management outside of FileStream.

@davidfowl
Copy link
Member

That strategy should work on all platforms (or it's a bug). You pass a buffer to FileStream anyways. Why would you want to have it use the array pool rather than do that yourself? It's much better and you can pre-pin buffers to avoid fragmentation.

I don't think we should add new overloads.

@sakno
Copy link
Contributor Author

sakno commented May 6, 2021

That's why overload with Memory<byte> parameter looks better that overloads with pools. Moving buffer management outside of FileStream increases complexity of my code: I need to track Position and Seek calls to flush the buffer when needed. All this stuff is completely done in Strategy abstraction which requires minimal modifications to support Memory<byte>.

@sakno
Copy link
Contributor Author

sakno commented May 6, 2021

Btw, the current implementation in .NET 6 doesn't use pinned arrays. new byte[_bufferLength] can be replaced with GC.AllocUninitializedArray<byte>(_bufferLength, true)

@scalablecory
Copy link
Contributor

Btw, the current implementation in .NET 6 doesn't use pinned arrays. new byte[_bufferLength] can be replaced with GC.AllocUninitializedArray<byte>(_bufferLength, true)

pre-pinned arrays are placed into Gen2 so we must be careful to only use them when we expect the array to last a while.

@davidfowl
Copy link
Member

They're placed in the POH actually which is different than Gen2 and this would be a bad usage of the POH.

@jkotas
Copy link
Member

jkotas commented May 6, 2021

It is different than Gen2 internally in the GC. From the outside, you cannot tell the difference. POH allocations put pressure on Gen2 GCs with the current implementation. #51363 (comment)

@davidfowl
Copy link
Member

Yea, I was being pedantic 😄

@adamsitnik
Copy link
Member

I've created a new proposal related to this feature request: #52446

@sakno
Copy link
Contributor Author

sakno commented May 7, 2021

OK, we can move any further discussion to one place. Closing issue.

@sakno sakno closed this as completed May 7, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.IO untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

No branches or pull requests

5 participants