FileSystem.Unix: Directory.Delete: remove per item syscall.

[tmds]

By recursing using FileSystemEnumerable we know the file type and
can omit the stat calls made by DirectoryInfo.Exists.

For the top level path, we can omit the call also and handle
non-directories when rmdir errno is ENOTDIR.

For the recursive case we can avoid recursion when the top level path rmdir
succeeds immediately.

FileSystemEntry is updated so IsSymbolicLink remembers the file is symbolic link
and does not make a syscall for it.

[ghost]

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

Issue Details

---

By recursing using FileSystemEnumerable we know the file type and
can omit the stat calls made by DirectoryInfo.Exists.

For the top level path, we can omit the call also and handle
non-directories when rmdir errno is ENOTDIR.

For the recursive case we can avoid recursion when the top level path rmdir
succeeds immediately.

FileSystemEntry is updated so IsSymbolicLink remembers the file is symbolic link
and does not make a syscall for it.

Author:	tmds
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[tmds]

I kept ENOENT for compatibility. ENOTDIR would be more appropriate since something exists.

[tmds]

@adamsitnik I would prefer to change this error code because I think the previous behavior was unintentional.
If you try to delete a socket file using Directory.Delete, the code used to throw ENOENT.
I think it is more appropriate to throw ENOTDIR. Do you agree?

[tmds]

@adamsitnik can you share your opinion?

[tmds]

CI fails are not related: RegexMatchTests.Match_ExcessPrefix(engine: SourceGenerated) times out on Windows and Linux.

[tmds]

Some changes overlap with #60214. Please review that first, and I'll rebase here to remove the overlap.

[adamsitnik]

As soon as we merge #60214 I am going to review this PR.

[tmds]

I've rebased and removed the overlap with #60214. This is up for review.

[carlossanlop]

Looks good so far. I left some comments for you to consider. Thanks for the change, @tmds.

Can you please run the enumeration benchmarks to ensure there's no perf regression?

[tmds]

@adamsitnik @carlossanlop I've renamed the methods and added some comments. I hope it is more clear now what the implementation does. I still need to add the test.

[adamsitnik]

@tmds thank you for another great contribution!

I've benchmarked this PR for removing a copy of a dotnet/runtime repository with all the build artifacts.

On average it's 8% faster and allocates 50% less memory:

Method	Job	Mean	Ratio	Allocated
RecursiveDeleteDirectory	before	2.550 s	1.00	78 MB
RecursiveDeleteDirectory	after	2.356 s	0.92	34 MB

Just out of curiosity I've compared .NET to the the rm core utility, .NET is still 10-20% slower. Most likely because rm is not using recursive code (it uses fts):

https://github.com/coreutils/coreutils/blob/3e2d64448300ea91be258af5b64ed3b97b865bd6/src/rm.c#L17-L19

But the implementation is non-trivial ;)

[tmds]

@carlossanlop I've updated the test so it also runs against the DirectoryInfo.Delete API.
The PR should now be good to merge (when the CI run finishes).

[adamsitnik]

@tmds some of the tests are failing now. Could you PTAL?

[tmds]

@adamsitnik it seems that this fails on Windows. Is that expected?

            var target = GetTestFilePath();
            Directory.CreateDirectory(target);

            var linkParent = GetTestFilePath();
            Directory.CreateDirectory(linkParent);

            var linkPath = Path.Combine(linkParent, GetTestFileName());
            Directory.CreateSymbolicLink(linkPath, target);

            Assert.True(Directory.Exists(linkPath), "linkPath should exist");

[adamsitnik]

it seems that this fails on Windows. Is that expected?

It's not expected to me, but I am not a symlink expert. @carlossanlop @jozkee is that expected?

@tmds Since you have not touched the Windows part, could you please just mark this test as Unix-specific for now so we can merge the PR?

[tmds]

It's not expected to me, but I am not a symlink expert. @carlossanlop @jozkee is that expected?

nvmd, I had unintentionally made changes to another similar test which caused it to fail.

The test should pass now on Linux and Windows.