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

Cache GetFileInformationByHandleEx (Length) when FileShare indicates that no one else can change it #49638

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
30d8ef7
introduce WindowsFileStreamStrategy
adamsitnik Feb 25, 2021
5c8018d
introduce SyncWindowsFileStreamStrategy
adamsitnik Feb 25, 2021
2e159c4
introduce AsyncWindowsFileStreamStrategy
adamsitnik Feb 25, 2021
5e87d72
some minor improvements after reading the code again
adamsitnik Feb 26, 2021
f5c2470
only DerivedFileStreamStrategy needs to have a reference to FileStream
adamsitnik Feb 26, 2021
71e7d43
implement ReadByte and WriteByte to make new windows strategies fully…
adamsitnik Feb 26, 2021
367ad84
implement FlushAsync for no buffering strategies as nop to get Flush_…
adamsitnik Feb 26, 2021
5579e94
use the new strategies when buffering is not enabled
adamsitnik Feb 26, 2021
8bbbcee
introduce BufferedFileStreamStrategy
adamsitnik Feb 26, 2021
9e9cb1f
fix the Mono build
adamsitnik Mar 1, 2021
53752e7
use the Legacy strategy by default for now, add tests for the other i…
adamsitnik Mar 2, 2021
e2eec8b
restore old file name to make it easier to review the code (as diff w…
adamsitnik Mar 2, 2021
20640fa
Don't set the buffer to null, to avoid a NullReferenceException
adamsitnik Mar 2, 2021
dd7e9ba
fix the browser build?
adamsitnik Mar 2, 2021
2a35ae6
reverting the flushing changes as it looks that they have introduced …
adamsitnik Mar 2, 2021
b9afbff
simplify the source code by removing some of the optimizations that a…
adamsitnik Mar 3, 2021
2029317
don't verify OS handle position
adamsitnik Mar 4, 2021
f6c8647
track the file offset in memory, don't use expensive sys calls to syn…
adamsitnik Mar 4, 2021
60eedb4
there is no need to set the Length since we are now tracking the offs…
adamsitnik Mar 5, 2021
72a4e94
Cache GetFileInformationByHandleEx (Length) when FileShare does not a…
jozkee Mar 15, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ private unsafe Task WriteAsyncInternalCore(ReadOnlyMemory<byte> source, Cancella
// touch the file pointer location at all. We will adjust it
// ourselves, but only in memory. This isn't threadsafe.
_filePosition += source.Length;
UpdateLengthOnChangePosition();
}

// queue an async WriteFile operation and pass in a packed overlapped
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace System.IO
public class FileStream : Stream
{
internal const int DefaultBufferSize = 4096;
private const FileShare DefaultShare = FileShare.Read;
internal const FileShare DefaultShare = FileShare.Read;
private const bool DefaultIsAsync = false;

/// <summary>Caches whether Serialization Guard has been disabled for file writes</summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ private unsafe void WriteSpan(ReadOnlySpan<byte> source)
}
Debug.Assert(r >= 0, "FileStream's WriteCore is likely broken.");
_filePosition += r;
return;
UpdateLengthOnChangePosition();
}

private NativeOverlapped GetNativeOverlappedForCurrentPosition()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy
/// <summary>Whether the file is opened for reading, writing, or both.</summary>
private readonly FileAccess _access;

private readonly FileShare _share;

/// <summary>The path to the opened file.</summary>
protected readonly string? _path;

Expand All @@ -32,6 +34,7 @@ internal abstract class WindowsFileStreamStrategy : FileStreamStrategy
private readonly bool _isPipe; // Whether to disable async buffering code.

private long _appendStart; // When appending, prevent overwriting file.
private long? _length;
Copy link
Member

Choose a reason for hiding this comment

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

what would be the difference in the allocated memory if we would use non-nullable long field and use negative values instead null to determine whether the field was initialized or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

This field doesn't actually allocate for being nullable; Nullable<T> is a struct, but it makes sense to use negatives instead.

Copy link
Member

Choose a reason for hiding this comment

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

This field doesn't actually allocate for being nullable

Is sizeof(long) == sizeof(long?)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is sizeof(long) == sizeof(long?)?

Nope, it is 8 and 16 respectively on my machine. I thought you were referring to heap allocations, I will prefer long for the upcoming pull request, thanks.


internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access)
{
Expand All @@ -40,6 +43,7 @@ internal WindowsFileStreamStrategy(SafeFileHandle handle, FileAccess access)
// Note: Cleaner to set the following fields in ValidateAndInitFromHandle,
// but we can't as they're readonly.
_access = access;
_share = FileStream.DefaultShare;
Copy link
Member

Choose a reason for hiding this comment

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

instead of using FileStream.DefaultShare here you can add FileShare share to the ctor arguments and pass FileStream.DefaultShare from FileStream. We are already doing similar thing:

this(path, mode, access, DefaultShare, DefaultBufferSize, DefaultIsAsync)


// As the handle was passed in, we must set the handle field at the very end to
// avoid the finalizer closing the handle when we throw errors.
Expand All @@ -52,6 +56,7 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access

_path = fullPath;
_access = access;
_share = share;

_fileHandle = FileStreamHelpers.OpenHandle(fullPath, mode, access, share, options);

Expand All @@ -77,7 +82,25 @@ internal WindowsFileStreamStrategy(string path, FileMode mode, FileAccess access

public sealed override bool CanWrite => !_fileHandle.IsClosed && (_access & FileAccess.Write) != 0;

public unsafe sealed override long Length => FileStreamHelpers.GetFileLength(_fileHandle, _path);
public unsafe sealed override long Length => _share > FileShare.Read ?
FileStreamHelpers.GetFileLength(_fileHandle, _path) :
_length ??= FileStreamHelpers.GetFileLength(_fileHandle, _path);

protected void UpdateLengthOnChangePosition()
{
// Do not update the cached length if the file can be written somewhere else
// or if the length has not been queried.
if (_share > FileShare.Read || _length is null)
{
Debug.Assert(_length is null);
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
return;
}

if (_filePosition > _length)
{
_length = _filePosition;
}
}

/// <summary>Gets or sets the position within the current stream</summary>
public override long Position
Expand Down Expand Up @@ -256,6 +279,7 @@ protected unsafe void SetLengthCore(long value)
Debug.Assert(value >= 0, "value >= 0");

FileStreamHelpers.SetLength(_fileHandle, _path, value);
carlossanlop marked this conversation as resolved.
Show resolved Hide resolved
_length = value;

if (_filePosition > value)
{
Expand Down