Skip to content

Fix Issue #21678: Add Path.Exists -- to replace inefficient FileExists || DirectoryExists - #64347

Merged
adamsitnik merged 13 commits into
dotnet:mainfrom
Tarun047:issue-21678
Jan 31, 2022
Merged

Fix Issue #21678: Add Path.Exists -- to replace inefficient FileExists || DirectoryExists #64347
adamsitnik merged 13 commits into
dotnet:mainfrom
Tarun047:issue-21678

Conversation

@Tarun047

Copy link
Copy Markdown
Contributor

Added a new API Path.Exists(...) as per the approved spec described in issue #21678.
This API is effectively equivalent to checking File.Exists(...) combined with Directory.Exists(...)
Using this API we only make one syscall to the kernel, which would have been two calls otherwise.

@ghostghost added community-contribution Indicates that the PR has been added by a community member area-System.IO new-api-needs-documentation labels Jan 26, 2022
@ghost

Copy link
Copy Markdown

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

Added a new API Path.Exists(...) as per the approved spec described in issue #21678.
This API is effectively equivalent to checking File.Exists(...) combined with Directory.Exists(...)
Using this API we only make one syscall to the kernel, which would have been two calls otherwise.

Author:Tarun047
Assignees:-
Labels:

area-System.IO, new-api-needs-documentation, community-contribution

Milestone:-

@dnfadmin

dnfadmin commented Jan 26, 2022

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

Comment threadsrc/libraries/System.IO.FileSystem/tests/Path/Exists.cs Outdated
Comment threadsrc/libraries/System.IO.FileSystem/tests/Path/Exists.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs Outdated
Comment threadsrc/libraries/System.IO.FileSystem/tests/Path/Exists.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs Outdated
@danmoseley

Copy link
Copy Markdown
Contributor

thanks, I added some comments.

@danmoseley

Copy link
Copy Markdown
Contributor

BTW @Tarun047 I'm guessing you're running on Windows and relying on PR validation to run the Unix tests. What many of us do that work on Windows is build and run tests on WSL2 (any distro eg., Ubuntu). It works really well. What I find easiest is to clone to my WSL2 distro exactly as I would on Windows, and set up the remote to my fork as I would on Windows, then push and pull via that to share bits. But some people set up remotes directly between Windows and WSL2. Once you have a clone, you just follow the regular doc in this repo for building and running tests on Linux.

Given you've already created a PR for your branch here, if I wanted to share bits between the two without kicking validation again, I would push from one to a new branch, and pull that branch down to the other. Verify tests pass in both, then push that branch to this PR branch.

@danmoseley

Copy link
Copy Markdown
Contributor

(Because relying on PR validation to run the tests for you makes development slow and noisy. We generally use PR validation to ensure that all the configuration matrix is tested, once we've run the key configuration(s) locally.)

@Tarun047

Copy link
Copy Markdown
ContributorAuthor

Yes @danmoseley I am using Windows machine for the primary development, I do have a M1 Macbook pro, but haven't found time to setup the build infrastructure on it yet, hopefully will do it soon.
Anyway thanks for the tip, I'd use a separate branches if I wanted to share code between 2 machines from next time.

@danmoseley

Copy link
Copy Markdown
Contributor

Cool, yes, I think you'd find it more productive for you and also less time burned on validation (which I think consumes 50-100 machines each push) 😸

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Tarun047 big thanks for your contribution!

I've added few comments, mostly style but also about adding triple slash comments.

When adding the triple slash comments you can use some of the existing text from https://docs.microsoft.com/en-us/dotnet/api/system.io.file.exists?view=net-6.0

Comment threadsrc/libraries/System.IO.FileSystem/tests/Path/Exists_File.cs Outdated
Comment threadsrc/libraries/System.IO.FileSystem/tests/Path/Exists_Directory.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Windows.cs Outdated
@adamsitnikadamsitnik added this to the 7.0.0 milestone Jan 28, 2022
@adamsitnik

Copy link
Copy Markdown
Member

FWIW I've benchmarked the two sys-calls that allow us to check whether given file exists or not and the current implementation of the PR is already using the optimal one. 👍

MethodIsFileExistsMeanErrorStdDevRatioRatioSD
GetFileAttributesExFalseFalse7.629 us0.1387 us0.0076 us1.000.00
FindFirstFileFalseFalse20.274 us1.8564 us0.1018 us2.660.01
GetFileAttributesExFalseTrue10.898 us0.5784 us0.0317 us1.000.00
FindFirstFileFalseTrue20.971 us1.5526 us0.0851 us1.920.01
GetFileAttributesExTrueFalse7.427 us1.6298 us0.0893 us1.000.00
FindFirstFileTrueFalse20.477 us0.6854 us0.0376 us2.760.03
GetFileAttributesExTrueTrue14.269 us0.1665 us0.0091 us1.000.00
FindFirstFileTrueTrue20.641 us0.7453 us0.0409 us1.450.00
Details
<ProjectSdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net6.0</TargetFramework>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
</PropertyGroup>
<ItemGroup>
<PackageReferenceInclude="BenchmarkDotNet"Version="0.13.1" />
</ItemGroup>
</Project>
usingBenchmarkDotNet.Attributes;usingBenchmarkDotNet.Running;usingMicrosoft.Win32.SafeHandles;usingSystem;usingSystem.IO;usingSystem.Runtime.InteropServices;namespaceFileExistsWindowsSyscalls{internalclassProgram{staticvoidMain(string[]args)=>BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);}publicclassSyscalls{string_path;[Params(true,false)]publicboolIsFile;[Params(true,false)]publicboolExists;[GlobalSetup]publicvoidSetup(){_path=Path.Combine(Path.GetTempPath(),Path.GetRandomFileName());if(Exists){if(IsFile)File.Create(_path).Dispose();elseDirectory.CreateDirectory(_path);}}[GlobalCleanup]publicvoidCleanup(){if(Exists){if(IsFile)File.Delete(_path);elseDirectory.Delete(_path);}}[Benchmark(Baseline=true)]publicboolGetFileAttributesEx(){WIN32_FILE_ATTRIBUTE_DATAdata=default;returnGetFileAttributesEx(_path,GET_FILEEX_INFO_LEVELS.GetFileExInfoStandard,refdata);}[Benchmark]publicboolFindFirstFile(){WIN32_FIND_DATAdata=default;usingSafeFindHandlehandle=FindFirstFileEx(_path,FINDEX_INFO_LEVELS.FindExInfoBasic,refdata,FINDEX_SEARCH_OPS.FindExSearchNameMatch,IntPtr.Zero,0);return!handle.IsInvalid;}[DllImport("kernel32.dll",EntryPoint="GetFileAttributesExW",CharSet=CharSet.Unicode,ExactSpelling=true,SetLastError=true)]privatestaticexternboolGetFileAttributesEx(stringname,GET_FILEEX_INFO_LEVELSfileInfoLevel,refWIN32_FILE_ATTRIBUTE_DATAlpFileInformation);[DllImport("kernel32.dll",EntryPoint="FindFirstFileExW",CharSet=CharSet.Unicode,ExactSpelling=true,SetLastError=true)]privatestaticexternSafeFindHandleFindFirstFileEx(stringlpFileName,FINDEX_INFO_LEVELSfInfoLevelId,refWIN32_FIND_DATAlpFindFileData,FINDEX_SEARCH_OPSfSearchOp,IntPtrlpSearchFilter,intdwAdditionalFlags);}internalenumGET_FILEEX_INFO_LEVELS:uint{GetFileExInfoStandard=0x0u,GetFileExMaxInfoLevel=0x1u,}internalstructFILE_TIME{internaluintdwLowDateTime;internaluintdwHighDateTime;internalFILE_TIME(longfileTime){dwLowDateTime=(uint)fileTime;dwHighDateTime=(uint)(fileTime>>32);}internallongToTicks()=>((long)dwHighDateTime<<32)+dwLowDateTime;internalDateTimeToDateTimeUtc()=>DateTime.FromFileTimeUtc(ToTicks());internalDateTimeOffsetToDateTimeOffset()=>DateTimeOffset.FromFileTime(ToTicks());}[StructLayout(LayoutKind.Sequential,CharSet=CharSet.Unicode)]internalunsafestructWIN32_FIND_DATA{internalconstintMAX_PATH=260;internaluintdwFileAttributes;internalFILE_TIMEftCreationTime;internalFILE_TIMEftLastAccessTime;internalFILE_TIMEftLastWriteTime;internaluintnFileSizeHigh;internaluintnFileSizeLow;internaluintdwReserved0;internaluintdwReserved1;privatefixedchar_cFileName[MAX_PATH];privatefixedchar_cAlternateFileName[14];internalReadOnlySpan<char>cFileName{get{ fixed (char*c=_cFileName)returnnewReadOnlySpan<char>(c,MAX_PATH);}}}internalstructWIN32_FILE_ATTRIBUTE_DATA{internalintdwFileAttributes;internalFILE_TIMEftCreationTime;internalFILE_TIMEftLastAccessTime;internalFILE_TIMEftLastWriteTime;internaluintnFileSizeHigh;internaluintnFileSizeLow;internalvoidPopulateFrom(refWIN32_FIND_DATAfindData){dwFileAttributes=(int)findData.dwFileAttributes;ftCreationTime=findData.ftCreationTime;ftLastAccessTime=findData.ftLastAccessTime;ftLastWriteTime=findData.ftLastWriteTime;nFileSizeHigh=findData.nFileSizeHigh;nFileSizeLow=findData.nFileSizeLow;}}internalenumFINDEX_INFO_LEVELS:uint{FindExInfoStandard=0x0u,FindExInfoBasic=0x1u,FindExInfoMaxInfoLevel=0x2u,}internalenumFINDEX_SEARCH_OPS:uint{FindExSearchNameMatch=0x0u,FindExSearchLimitToDirectories=0x1u,FindExSearchLimitToDevices=0x2u,FindExSearchMaxSearchOp=0x3u,}internalsealedclassSafeFindHandle:SafeHandleZeroOrMinusOneIsInvalid{publicSafeFindHandle():base(true){}protectedoverrideboolReleaseHandle()=>FindClose(handle);[DllImport("kernel32.dll",SetLastError=true)]internalstaticexternboolFindClose(IntPtrhFindFile);}}

@adamsitnikadamsitnik self-assigned this Jan 28, 2022

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've added some suggestions for how we could improve the docs. PTAL.

Comment threadsrc/libraries/System.IO.FileSystem/tests/Directory/Exists.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.cs Outdated
Comment threadsrc/libraries/System.Private.CoreLib/src/System/IO/Path.cs
Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com>

@adamsitnikadamsitnik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks very good to me! Thank you for your contribution @Tarun047 !

@adamsitnik
adamsitnik merged commit c712129 into dotnet:mainJan 31, 2022
@danmoseley

Copy link
Copy Markdown
Contributor

@Tarun047 we have more issues marked up for grabs - perhaps there is another that is interesting to you?

@Tarun047

Copy link
Copy Markdown
ContributorAuthor

Sure

@ghostghost locked as resolved and limited conversation to collaborators Mar 3, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.IOcommunity-contributionIndicates that the PR has been added by a community membernew-api-needs-documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Tarun047@dnfadmin@danmoseley@adamsitnik