fix SafeFileHandle.CanSeek performance regression#125807
fix SafeFileHandle.CanSeek performance regression#125807adamsitnik wants to merge 1 commit intomainfrom
Conversation
|
@EgorBot -windows_intel using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
BenchmarkSwitcher.FromAssembly(typeof(Benchmarks).Assembly).Run(args);
[MemoryDiagnoser]
public class Benchmarks
{
private string? _filePath;
[GlobalSetup]
public void Setup()
{
_filePath = Path.GetTempFileName();
File.WriteAllBytes(_filePath, new byte[1_000]);
}
[GlobalCleanup]
public void Cleanup() => File.Delete(_filePath!);
[Benchmark]
public byte[] ReadAllBytes() => File.ReadAllBytes(_filePath!);
[Benchmark]
public Task ReadAllBytesAsync() => File.ReadAllBytesAsync(_filePath!);
[Benchmark]
public bool OpenAndCanSeek()
{
using FileStream handle = File.OpenRead(_filePath!);
return handle.CanSeek;
}
} |
|
Tagging subscribers to this area: @dotnet/area-system-io |
There was a problem hiding this comment.
Pull request overview
This PR targets a Windows performance regression in SafeFileHandle.CanSeek / SafeFileHandle.Type usage by avoiding an extra handle-type probe for common disk-file handles.
Changes:
- Adds a
GetFileTypeCorefast-path forFILE_TYPE_DISKhandles whenPathis available, returningRegularFilewithout callingGetDiskBasedType().
| // which does not allow opening directories or symbolic links (it resolves them). | ||
| // That is why we can skip the extra check performed by GetDiskBasedType. | ||
| Interop.Kernel32.FileTypes.FILE_TYPE_DISK when Path is not null => FileHandleType.RegularFile, |
There was a problem hiding this comment.
The new fast-path assumes Path is not null implies the handle can't be a directory or a symlink, but SafeFileHandle.Open passes through FileOptions and FileStreamHelpers.ValidateArguments explicitly allows (FileOptions)0x02000000 (FILE_FLAG_BACKUP_SEMANTICS). With that flag, CreateFile can successfully open directory handles while still setting _path, so this switch arm would misclassify such handles as RegularFile (making Type/CanSeek incorrect). Consider narrowing the fast-path to cases where FILE_FLAG_BACKUP_SEMANTICS is not set (or otherwise proving directories can’t be opened) and adjust the comment accordingly.
| // which does not allow opening directories or symbolic links (it resolves them). | |
| // That is why we can skip the extra check performed by GetDiskBasedType. | |
| Interop.Kernel32.FileTypes.FILE_TYPE_DISK when Path is not null => FileHandleType.RegularFile, | |
| // which does not allow opening directories or symbolic links (it resolves them) unless | |
| // FILE_FLAG_BACKUP_SEMANTICS (0x02000000) is specified in FileOptions to allow opening directories. | |
| // When backup semantics are not used, we can skip the extra check performed by GetDiskBasedType. | |
| Interop.Kernel32.FileTypes.FILE_TYPE_DISK when Path is not null && (GetFileOptions() & (FileOptions)0x02000000) == 0 => FileHandleType.RegularFile, |
fixes #125660