Skip to content

Conversation

AR-May
Copy link
Member

@AR-May AR-May commented Aug 18, 2021

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

  • Added Microsoft.IO.Redist
  • Default file system enumeration uses Microsoft.IO instead of System.IO

Testing

Unit tests & DDRITs

Notes

We should be wary of possible regressions. There were differences in behavior, see .NET Framework only notes in doc. The change therefore is under change wave 17_0.

@AR-May
Copy link
Member Author

AR-May commented Aug 18, 2021

I measured CPU and memory of the enumeration requests from FileMatcher.cs functions for 3 cases.

  • Enumeration uses old API from .NET Framework System.IO.
  • Enumeration uses old API from Microsoft.IO
  • New enumeration, using the new API from Microsoft.IO

Measurements are done for the rebuild of OrchardCore repo, it is average of 3 attempts.

Description CPU (msec) Memory (bytes)
System.IO old API 9996 63933
Microsoft.IO old API 4656 44133
Microsoft.IO new API 3927 52133

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.

@AR-May AR-May marked this pull request as ready for review August 18, 2021 14:06
@AR-May AR-May marked this pull request as draft August 18, 2021 14:39
Copy link
Contributor

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

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.

@AR-May
Copy link
Member Author

AR-May commented Sep 3, 2021

So far, it seems that I was able to resolve problems with the VS experimental insertion.
The absence of the binding redirect was indeed the root cause of the failures that I saw (but not for Microsoft.IO.Redist, for one of the dependency assemblies).
The actual exception "Could not load file or assembly 'System.Buffers, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The system cannot find the file specified." was covered with a nonsense error "System.ArgumentException: Illegal characters in path." and mislead me.

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.

@AR-May AR-May force-pushed the add-microsoft-io-redist-3 branch from 6795cc8 to c80abab Compare September 8, 2021 09:40
@AR-May AR-May marked this pull request as ready for review September 20, 2021 08:43
Copy link
Contributor

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

I know you said there are still errors, but the code itself LGTM 👍

@AR-May AR-May force-pushed the add-microsoft-io-redist-3 branch from 4832fa9 to 1a37ff2 Compare October 1, 2021 13:48
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

:shipit:

@Forgind Forgind added merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. and removed merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. labels Oct 8, 2021
@AR-May AR-May force-pushed the add-microsoft-io-redist-3 branch from d94c76b to ea01e3c Compare October 13, 2021 14:33
@AR-May AR-May marked this pull request as draft October 13, 2021 14:42
@AR-May AR-May force-pushed the add-microsoft-io-redist-3 branch from ea01e3c to 334fb53 Compare October 14, 2021 11:42
@AR-May AR-May force-pushed the add-microsoft-io-redist-3 branch from 334fb53 to d6b2109 Compare October 14, 2021 11:46
@AR-May AR-May marked this pull request as ready for review October 14, 2021 12:48
Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Looks awesome! Thanks for all the work it took to get to this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Performance merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Switch System.IO usage with Microsoft.IO usage to reduce string allocations during scanning directory (and some other path manipulation too.)

4 participants