Add Microsoft.IO.Redist for directory enumeration.#6771
Add Microsoft.IO.Redist for directory enumeration.#6771ladipro merged 10 commits intodotnet:mainfrom
Conversation
|
I measured CPU and memory of the enumeration requests from FileMatcher.cs functions for 3 cases.
Measurements are done for the rebuild of OrchardCore repo, it is average of 3 attempts.
Considering that the difference is not that big and implementation with old API is much easier (no need to change in our public interfaces or work around it) we decided to use old API from Microsoft.IO. |
824746b to
6795cc8
Compare
Forgind
left a comment
There was a problem hiding this comment.
I marked the couple things I think are most likely to be the problem. I also see extra binding redirects for some other assemblies in the VS repo that aren't there for M.IO.Redist, so that's a possibility. I haven't found anything that pinpoints the problem, unfortunately, so I can't say that any of them are actually the problem.
|
So far, it seems that I was able to resolve problems with the VS experimental insertion. The other errors that I saw should be flukes or not related, as far as I could see. I will run the final version through the experimental insertion one more time to confirm resolution of the errors. |
6795cc8 to
c80abab
Compare
Forgind
left a comment
There was a problem hiding this comment.
I know you said there are still errors, but the code itself LGTM 👍
4832fa9 to
1a37ff2
Compare
ladipro
left a comment
There was a problem hiding this comment.
I have left a few comments inline, thank you!
| throw new InvalidOperationException("Could not load file or assembly.", ex); | ||
| } | ||
| // Sometimes FileNotFoundException is thrown when there is an assembly load failure. In this case it should have FusionLog. | ||
| catch (FileNotFoundException ex) when (ex.FusionLog != null) |
There was a problem hiding this comment.
Isn't the FusionLog property available only if fusion logging is actually enabled?
I don't see FileNotFoundException listed as an exception thrown by Directory.Enumerate* so it looks like it should work without the when.
There was a problem hiding this comment.
It works when fusion logging is disabled. The FusionLog in this case consists of a warning "WRN: Assembly binding logging is turned OFF. To enable assembly bind failure logging, set the registry value [HKLM\Software\Microsoft\Fusion!EnableLog] (DWORD) to 1."
If Directory.Enumerate indeed does not throw FileNotFoundException, then additional check in when should not have any performance implications, cause it is used in rare case when we indeed have a failure. And I felt like it is safer to have it than not not have.
But I suppose you are right and we can remove this when from this line.
d94c76b to
ea01e3c
Compare
ea01e3c to
334fb53
Compare
334fb53 to
d6b2109
Compare
rainersigwald
left a comment
There was a problem hiding this comment.
Looks awesome! Thanks for all the work it took to get to this point.
Fixes #6075
Context
Microsoft.IO.Redist brings some of the new .NET Core System.IO functionality to .NET Framework. In particular, enumeration in Microsoft.IO was optimized comparing to System.IO: new enumeration API was added and old one was improved. We consider it is beneficial to switch default file system enumeration to the old API from Microsoft.IO.Redist.
Changes Made
Testing
Unit tests & DDRITs
Notes
We should be wary of possible regressions. There were differences in behavior, see
.NET Framework onlynotes in doc. The change therefore is under change wave 17_0.