Add tracking a recursion depth in System.IO.FileSystem by iSazonov · Pull Request #48148 · dotnet/runtime

iSazonov · 2021-02-11T03:43:26Z

ghost · 2021-02-11T03:43:33Z

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.

carlossanlop

Thanks for your PR, @iSazonov! I left some comments.

carlossanlop · 2021-02-16T19:50:03Z

src/libraries/System.IO.FileSystem/src/System/IO/EnumerationOptions.cs

+        /// <summary>
+        /// Stop recursion if current depth exceeds the value.
+        /// Default is int.MaxValue.
+        /// </summary>


Since we are working on making triple slash comments the source of truth for documentation, we need to give a proper style and language review here before commiting, so it's ready to get ported to MS Docs. So below is my suggestion.

cc @gewarren in case she thinks my suggestion can be improved:

Suggested change

/// <summary>

/// Stop recursion if current depth exceeds the value.

/// Default is int.MaxValue.

/// </summary>

/// <summary>Gets or sets a value that indicates the maximum directory depth to recurse while enumerating, when <see cref="RecurseSubdirectories" /> is set to <see langword="true" />.</summary>

/// <value>A number that represents the maximum directory depth to recurse while enumerating. The default value is <see cref="int.MaxValue" />.</value>

/// <remarks>If <see cref="MaxRecursionDepth" /> is set to a negative number, the default value <see cref="int.MaxValue" /> is used.

/// If <see cref="MaxRecursionDepth" /> is set to zero, returns the contents of the initial directory.</remarks>

We could add in remarks about zero value too:

/// <remarks>If <see cref="MaxRecursionDepth" /> is set to a negative number, the default value <see cref="int.MaxValue" /> is used. If <see cref="MaxRecursionDepth" /> is set to zero, returns the contents of the initial directory.</remarks>

Waiting @gewarren ...

@iSazonov I added your extra zero value suggestion to my comment. You can commit the suggestion if you agree. @gewarren is OOF but she can let us know later if we should modify it.

For sharing experience. In PowerShell repo we do the work async to exclude PR merge delaying. We create and reference in PR description new doc issue (or new PR in PowerShell-Docs repo) so that add new docs after functional PR merged.
This approach resolve follow issues:

English is not native for the contributor

The contributor can not know all rules of creating docs

I'd change the second remark to something like:

If <see cref="MaxRecursionDepth" /> is set to zero, enumeration returns the contents of the initial directory.

carlossanlop · 2021-02-16T19:58:23Z

src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.cs

            _options = options ?? EnumerationOptions.Default;

+            _remainingDepth = _options.MaxRecursionDepth < 0 ? EnumerationOptions.Default.MaxRecursionDepth : _options.MaxRecursionDepth;
+


@adamsitnik do you know if it's costly to generate a default EnumerationOptions instance just to retrieve the default value of MaxRecursionDepth, as opposed to simply using the underlying default value?

@iSazonov I don't expect we will ever change the default value of MaxRecursionDepth, mainly because it's type (int) would never be changed.

I also noticed that line 41 is already consuming EnumerationOptions.Default, so there are two things we could do here:

A) Save EnumerationOptions.Default to a variable and then consume it in both places:

Suggested change

_options = options ?? EnumerationOptions.Default;

_remainingDepth = _options.MaxRecursionDepth < 0 ? EnumerationOptions.Default.MaxRecursionDepth : _options.MaxRecursionDepth;

var defaultOptions = EnumerationOptions.Default;

_options = options ?? defaultOptions;

_remainingDepth = _options.MaxRecursionDepth < 0 ? defaultOptions.MaxRecursionDepth : _options.MaxRecursionDepth;

or B) Use the default value directly:

Suggested change

_options = options ?? EnumerationOptions.Default;

_remainingDepth = _options.MaxRecursionDepth < 0 ? EnumerationOptions.Default.MaxRecursionDepth : _options.MaxRecursionDepth;

_options = options ?? EnumerationOptions.Default;

_remainingDepth = _options.MaxRecursionDepth < 0 ? int.MaxValue : _options.MaxRecursionDepth;

@iSazonov I don't expect we will ever change the default value of MaxRecursionDepth, mainly because it's type (int) would never be changed.

I believe it makes the code more readable if we use a sort of named constants.
Also I guess/hope the runtime will optimize the case.

Waiting @adamsitnik ...

+1 to use a named constant (perhaps called DefaultMaxRecursionDepth) inside EnumerationOptions and use it where its necessary.

carlossanlop · 2021-02-16T20:06:10Z

src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs

                        if (isDirectory && !isSpecialDirectory)
                        {
-                            if (_options.RecurseSubdirectories && ShouldRecurseIntoEntry(ref entry))
+                            if (_options.RecurseSubdirectories && _remainingDepth > 0 && ShouldRecurseIntoEntry(ref entry))


I know from experience that any changes in FileSystemEnumerator.MoveNext could cause ugly performance regressions.

Can you please run the enumeration performance benchmarks, before and after your changes, to see if there's an impact?

In case you've not run benchmarks before, here are the nstructions: dotnet/performance/docs/benchmarking-workflow-dotnet-runtime.md#preventing-regressions

These are the relevant benchmarks: dotnet/performance/src/benchmarks/micro/libraries/System.IO.FileSystem/Perf.Directory.cs.

I am particularly interested in the results for RecursiveDepthData() and EnumerateFiles().

I will try to run these tests, but it may take a long time since I have never done this.
Perhaps @adamsitnik will get ahead of me. :-)

After my first attempt no regression was found. I am not sure I did everything is correct.

Can you please share the results here?

I will do but I am not still sure I make right steps to make the work. Docs are not so clear about iterations. If an expert like @adamsitnik published a short (5 minutes) video with demo steps how clean start, make a base and iterate with changes it would be great help (because each iteration takes some hours on my notebook).

Currently I can share only "No differences found between the benchmark results with threshold 1%.". Do you want to get an archive with result files?

There is very good documentation in the dotnet/performance repo. You should end up with a file containing markdown tables that you can paste directly into your PR here.

Also it should take no more than a few minutes to run the relevant benchmarks. You can just filter to the file enumeration ones.

I extracted your zip file and compared the results using ResultComparer from the performance repo:

D:\performance\src\tools\ResultsComparer> dotnet run --base "C:\Users\carlos\Desktop\New folder\before" --diff "C:\Users\carlos\Desktop\New folder\after" --threshold 0.00001% summary: better: 1, geomean: 1.016 total diff: 1 No Slower results for the provided threshold = 0.00001% and noise filter = 0.3ns. | Faster | base/diff | Base Median (ns) | Diff Median (ns) | Modality| | --------------------------------------------- | ---------:| ----------------:| ----------------:| --------:| | System.IO.Tests.Perf_Directory.EnumerateFiles | 1.02 | 8712455.56 | 8576044.64 | |

@iSazonov can you run a benchmark test for the other enumeration-related method RecursiveDepthData()? Or even better: just run all the methods in that perf class.

@carlossanlop I don't see regressions but I can not trust myself and you can not too because I never run perf tests before (I am not sure I do all right). In any case we need a confirmation from more experienced developer.

All results for System.IO.Tests.Perf_Directory.*
2.zip

…tions.cs Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>

…epth-tracking

iSazonov · 2021-03-25T14:27:37Z

@adamsitnik Could you please review?

src/libraries/System.IO.FileSystem/src/System/IO/EnumerationOptions.cs

src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj

src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.Unix.cs

jozkee · 2021-03-25T16:24:57Z

src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.cs

            _rootDirectory = Path.TrimEndingDirectorySeparator(path);
            _options = options ?? EnumerationOptions.Default;

+            _remainingDepth = _options.MaxRecursionDepth < 0 ? EnumerationOptions.Default.MaxRecursionDepth : _options.MaxRecursionDepth;


I wonder if we should throw for negatives rather than take it as "use max value".

It seems strange to me that we define it as Int and then throw if negative. If we only want non-negative values then we should either be UInt or just use the default for them.
But I don't know that is a best practice in Runtime.

One reason for using signed over unsigned is CLS-compliance. Another one is that signed integers are already the standard across many .NET APIs e.g:

FileStream.SetLength(long value)
List<T>.Capacity
JsonSerializerOptions.MaxDepth

Notice that the APIs I linked guard against negatives. We should do the same here.

Thanks for samples! I added a value check in MaxRecursionDepth.

Done.

src/libraries/System.IO.FileSystem/tests/Enumeration/DepthTests.cs

jozkee · 2021-03-25T16:33:24Z

src/libraries/System.IO.FileSystem/src/System/IO/EnumerationOptions.cs

+        /// <summary>Gets or sets a value that indicates the maximum directory depth to recurse while enumerating, when <see cref="RecurseSubdirectories" /> is set to <see langword="true" />.</summary>
+        /// <value>A number that represents the maximum directory depth to recurse while enumerating. The default value is <see cref="int.MaxValue" />.</value>
+        /// <remarks>If <see cref="MaxRecursionDepth" /> is set to a negative number, the default value <see cref="int.MaxValue" /> is used.
+        /// If <see cref="MaxRecursionDepth" /> is set to zero, enumeration returns the contents of the initial directory.</remarks>
+        public int MaxRecursionDepth { get; set; }


indicates the maximum directory depth to recurse while enumerating, when is set to <see langword="true"

Can we consider throw when someone tries to set a value to this property and RecurseSubdirectories is false? It would make more clear that MaxRecursionDepth is not honored in that case.

In my opinion, it would make sense if we did not have the default, but we have regardless of the value of RecurseSubdirectories.

I can think of a few options to deal with this:

Keep MaxRecursionDepth as int.MaxValue and throw when set_MaxRecursionDepth is called if RecurseSubdirectories == false (what I already proposed in my previous comment).

Keep MaxRecursionDepth as default (zero) and change it to int.MaxValue when RecurseSubdirectories is set to true. Then we can throw when someone tries to call set_MaxRecursionDepth if RecurseSubdirectories == false (I feel that is not very intuitive that a property changes if another one does).

Do nothing and expect that the users have read the documentation so they can know that MaxRecursionDepth doesn't work on its own and that is only honored when RecurseSubdirectories == true (what we already have).

cc @carlossanlop @adamsitnik

I prefer third case. The API allows flipping RecurseSubdirectories on the fly so we would have very complex check code. Recursion is common scenario, checking depth is temporary one. If users turn off recursion and set MaxRecursionDepth to a value, they will not get a destructive result - again, there is always a default value, which also implies that there is nothing destructive there.

jozkee

@iSazonov It seems that the tests need to be updated based on the negative check you just introduced.
PS: I pushed to your branch in order to fix the build issues in CI.

jozkee · 2021-03-30T05:25:02Z

src/libraries/System.IO.FileSystem/src/System/IO/EnumerationOptions.cs

 {
    public class EnumerationOptions
    {
+        private int _maxRecursionDepth = DefaultMaxRecursionDepth;


For consistency can you please set this field in the ctor.

Ah, it is already in ctor :-)

Dup initialization removed.

into add-depth-tracking

iSazonov · 2021-03-30T08:58:25Z

@iSazonov It seems that the tests need to be updated based on the negative check you just introduced.
PS: I pushed to your branch in order to fix the build issues in CI.

I fixed the test.
Make sense to move the negative test to new file for EnumerationOptions class?

jozkee · 2021-03-30T23:29:50Z

@iSazonov not to a new file but to a separate unit test. Can you please do that?

jozkee

Implementation looks good to me, with the exception of a few nits. Only pending item is splitting the negative scenario to a separate test.

jozkee · 2021-03-30T23:41:19Z

src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.cs

 {
    public unsafe abstract partial class FileSystemEnumerator<TResult> : CriticalFinalizerObject, IEnumerator<TResult>
    {
+        private int _remainingDepth;


Could we rename this as _remainingRecursionDepth?

jozkee · 2021-03-30T23:43:09Z

src/libraries/System.IO.FileSystem/src/System/IO/EnumerationOptions.cs

        /// </summary>
        internal static EnumerationOptions Default { get; } = new EnumerationOptions();

+        internal const int DefaultMaxRecursionDepth = int.MaxValue;


Can you please move this constant to the beginning of the class (line 15)?

jozkee · 2021-03-30T23:43:55Z

src/libraries/System.IO.FileSystem/src/System/IO/Enumeration/FileSystemEnumerator.cs


+            _remainingDepth = _options.MaxRecursionDepth;


nit: whitespace.

Suggested change

_remainingDepth = _options.MaxRecursionDepth;

_remainingDepth = _options.MaxRecursionDepth;

jozkee

Assuming that CI passes this LGTM, thanks!

iSazonov · 2021-03-31T08:24:21Z

@jozkee Thanks for review! Could you please explain what Preview will get the API?

jozkee · 2021-03-31T09:12:48Z

@iSazonov it will be available in Preview 4.

Add tracking a depth recursion in System.IO.FileSystem

2631fa6

ghost added area-System.IO new-api-needs-documentation labels Feb 11, 2021

iSazonov mentioned this pull request Feb 11, 2021

Directory.Delete fails with IOException on Linux with very deep tree #41195

Closed

jozkee requested review from adamsitnik, carlossanlop and jozkee February 11, 2021 16:16

iSazonov changed the title ~~Add tracking a depth recursion in System.IO.FileSystem~~ Add tracking a recursion depth in System.IO.FileSystem Feb 12, 2021

carlossanlop reviewed Feb 16, 2021

View reviewed changes

carlossanlop added this to the Future milestone Feb 16, 2021

carlossanlop assigned iSazonov Feb 16, 2021

iSazonov and others added 3 commits February 18, 2021 11:31

Update src/libraries/System.IO.FileSystem/src/System/IO/EnumerationOp…

0c94f99

…tions.cs Co-authored-by: Carlos Sanchez <1175054+carlossanlop@users.noreply.github.com>

Merge branch 'master' of https://github.com/dotnet/runtime into add-d…

161561b

…epth-tracking

Fix docs

cc06bb2

Base automatically changed from master to main March 1, 2021 09:07

jozkee reviewed Mar 25, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/src/System/IO/EnumerationOptions.cs Outdated Show resolved Hide resolved

jozkee reviewed Mar 25, 2021

View reviewed changes

iSazonov and others added 4 commits March 27, 2021 20:26

Address feedbacks

006db4e

Fix typo

d57e1ec

Add MaxRecursionDepth value check

999bfb1

Fix net48 build: Add string to MS.IO.Redist resx file

2a427b5

jozkee reviewed Mar 30, 2021

View reviewed changes

iSazonov added 2 commits March 30, 2021 13:48

Fix test and initialization.

f1f12f4

Merge branch 'add-depth-tracking' of https://github.com/iSazonov/runtime

9b7a96b

into add-depth-tracking

jozkee reviewed Mar 30, 2021

View reviewed changes

Address feedback

b3a4f01

jozkee approved these changes Mar 31, 2021

View reviewed changes

jozkee merged commit cee9df8 into dotnet:main Mar 31, 2021

iSazonov deleted the add-depth-tracking branch March 31, 2021 08:24

ghost locked as resolved and limited conversation to collaborators Apr 30, 2021

		_options = options ?? EnumerationOptions.Default;

		_remainingDepth = _options.MaxRecursionDepth < 0 ? EnumerationOptions.Default.MaxRecursionDepth : _options.MaxRecursionDepth;

-            _options = options ?? EnumerationOptions.Default;
-            _remainingDepth = _options.MaxRecursionDepth < 0 ? EnumerationOptions.Default.MaxRecursionDepth : _options.MaxRecursionDepth;
+           var defaultOptions = EnumerationOptions.Default;
+            _options = options ?? defaultOptions;
+            _remainingDepth = _options.MaxRecursionDepth < 0 ? defaultOptions.MaxRecursionDepth : _options.MaxRecursionDepth;

Conversation

iSazonov commented Feb 11, 2021

Uh oh!

ghost commented Feb 11, 2021

Uh oh!

carlossanlop left a comment

Choose a reason for hiding this comment

Uh oh!

carlossanlop Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov commented Mar 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov Mar 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jozkee Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop Feb 16, 2021 •

edited

Loading

iSazonov Feb 17, 2021 •

edited

Loading

iSazonov Mar 27, 2021 •

edited

Loading

jozkee Mar 29, 2021 •

edited

Loading