Use System.Linq in fewer places #31199

pranavkm · 2021-03-24T17:32:04Z

Replace with alternatives or iteration where possible
Rewrite Router component to avoid allocations

pranavkm · 2021-03-24T20:10:25Z

javiercn · 2021-03-24T20:22:40Z

I'm in general OK with this, however this is somewhat of a loosing battle if we don't "ban" these APIs with an analyzer. I'm also a bit wary about how much of a win this is for real apps when it comes to trimming, since all libraries/customers need to also avoid these APIs for it to make a difference.

eerhardt · 2021-03-24T21:28:52Z

FYI @stephentoub - since you've done a lot of this work in dotnet/runtime.

src/Components/Authorization/src/AttributeAuthorizeDataCache.cs

src/Components/Components/src/Reflection/ComponentProperties.cs

src/Components/Components/src/ComponentFactory.cs

stephentoub · 2021-03-25T15:21:22Z

src/Components/Components/src/Reflection/MemberAssignment.cs

@@ -16,13 +15,13 @@ internal class MemberAssignment
            [DynamicallyAccessedMembers(Component)] Type type,
            BindingFlags bindingFlags)
        {
-            var dictionary = new Dictionary<string, List<PropertyInfo>>();
+            var dictionary = new Dictionary<string, List<PropertyInfo>>(StringComparer.Ordinal);


Why pass in the comparer explicitly? It's not a problem, I'm just wondering what it's fixing. Just for clarity?

A lot of dictionaries in ASP.NET Core tend to use or require an OrdinalIgnoreCase behavior. I prefer being explicit about it when we don't.

src/Components/Components/src/Reflection/MemberAssignment.cs

src/Components/Components/src/Routing/RouteKey.cs

src/Components/Components/src/Routing/RouteTableFactory.cs

stephentoub · 2021-03-25T15:43:21Z

src/Components/Components/src/Routing/Router.cs

            }
        }

+        private void RefreshRouteTableLegacy()
+        {
+            var assemblies = AdditionalAssemblies == null ? new[] { AppAssembly } : new[] { AppAssembly }.Concat(AdditionalAssemblies);


Does this need to compile against downlevel? If not, Prepend will be cheaper than new[] + Concat here.

I can update this, but I'm hopeful we could remove or trim this code: #29913

stephentoub · 2021-03-25T15:47:38Z

src/Components/Shared/src/ComponentParametersTypeCache.cs

@@ -29,8 +28,7 @@ internal class ComponentParametersTypeCache
        [RequiresUnreferencedCode("This type attempts to load component parameters that may be trimmed.")]
        private static Type? ResolveType(Key key, Assembly[] assemblies)
        {
-            var assembly = assemblies
-                .FirstOrDefault(a => string.Equals(a.GetName().Name, key.Assembly, StringComparison.Ordinal));
+            var assembly = Array.Find(assemblies, a => string.Equals(a.GetName().Name, key.Assembly, StringComparison.Ordinal));


This is still closing over key and allocating a delegate to boot. May as well avoid that, too :)

foreach (Assembly a in assemblies) { if (a.GetName().Name == key.Assembly) { return a.GetType(key.Type, throwOnError: false, ignoreCase: false); } } return null;

stephentoub · 2021-03-25T15:48:15Z

src/Components/Shared/src/RootComponentTypeCache.cs

@@ -29,8 +27,7 @@ internal class RootComponentTypeCache

        private static Type? ResolveType(Key key, Assembly[] assemblies)
        {
-            var assembly = assemblies
-                .FirstOrDefault(a => string.Equals(a.GetName().Name, key.Assembly, StringComparison.Ordinal));
+            var assembly = Array.Find(assemblies, a => string.Equals(a.GetName().Name, key.Assembly, StringComparison.Ordinal));


pranavkm · 2021-03-25T16:36:39Z

src/Components/Components/src/Reflection/MemberAssignment.cs

@@ -16,13 +15,13 @@ internal class MemberAssignment
            [DynamicallyAccessedMembers(Component)] Type type,
            BindingFlags bindingFlags)
        {
-            var dictionary = new Dictionary<string, List<PropertyInfo>>();
+            var dictionary = new Dictionary<string, List<PropertyInfo>>(StringComparer.Ordinal);


A lot of dictionaries in ASP.NET Core tend to use or require an OrdinalIgnoreCase behavior. I prefer being explicit about it when we don't.

src/Components/Components/src/Routing/RouteTableFactory.cs

pranavkm · 2021-03-25T16:41:42Z

src/Components/Components/src/Routing/Router.cs

            }
        }

+        private void RefreshRouteTableLegacy()
+        {
+            var assemblies = AdditionalAssemblies == null ? new[] { AppAssembly } : new[] { AppAssembly }.Concat(AdditionalAssemblies);


I can update this, but I'm hopeful we could remove or trim this code: #29913

src/Components/Authorization/src/AttributeAuthorizeDataCache.cs

pranavkm · 2021-03-25T16:53:11Z

src/Components/Components/src/Routing/RouteTableFactory.cs

            foreach (var componentType in componentTypes)
            {
                // We're deliberately using inherit = false here.
                //
                // RouteAttribute is defined as non-inherited, because inheriting a route attribute always causes an
                // ambiguity. You end up with two components (base class and derived class) with the same route.
                var routeAttributes = componentType.GetCustomAttributes<RouteAttribute>(inherit: false);
+                var templates = new List<string>(routeAttributes.Length);


Slightly annoying because I have to use the non-generic version of GetCustomAttributes to have it return an array. The generic one returns an IEnumerable

src/JSInterop/Microsoft.JSInterop/src/Microsoft.JSInterop.WarningSuppressions.xml

src/Components/Authorization/src/AttributeAuthorizeDataCache.cs

src/Components/Components/src/ComponentFactory.cs

SteveSandersonMS · 2021-03-30T14:02:32Z

src/Components/Components/src/Reflection/MemberAssignment.cs

+                var moveNext = _spanEnumerator.MoveNext();
+                Debug.Assert(moveNext, "We expect this to at least have one item.");
+                return moveNext;
+            }


This is a lot of extra machinery to reason about and maintain, especially with the caveats like "Do NOT make this readonly". People reading it may feel it could be done much more simply (e.g., the previous implementation) so it would be good to insert some kind of comment clarifying why we're doing all this.

How big is the payoff from doing all this? Bearing in mind that our plan is to move to a sourcegenerator-based way of setting component parameters, are we still going to be getting a meaningful benefit from doing this afterwards?

I definitely went overboard with this. It's a lot of code for what would amount to be a growingly smaller case (i.e. component libraries that were compiled without source generators). I'll turn this back into a foreach loop. It has one extra allocation, but I'd wager the readability is preferrable.

HaoK · 2021-03-30T21:12:22Z

src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHostConfiguration.cs

-                .Distinct(StringComparer.OrdinalIgnoreCase)
-                .Select(key => this.GetSection(key))
-                .ToList();
+            var hashSet = new HashSet<string>(StringComparer.OrdinalIgnoreCase);


Completely unrelated question for my own insight, why does WASM need to be able to build/modify configuration providers like this (i.e. why do we need a WASMHostConfiguration that deviates from the configure/build/immutable configuration pattern)

The host builder in WASM is a single shot operation similar to the feather http builder. You configure services on it and tell it to run. We found that you frequently need access to configuration as you're building it - hence it has this read / write behavior. I thought there was an issue filed by @davidfowl to add support for this by default in config, but I can't seem to find it.

@halter73 is gonna file it soon.

src/Components/Components/src/Reflection/ComponentProperties.cs

SteveSandersonMS · 2021-04-23T09:27:16Z

src/Components/Forms/src/EditContextDataAnnotationsExtensions.cs

@@ -67,7 +67,10 @@ private void OnFieldChanged(object? sender, FieldChangedEventArgs eventArgs)

                    Validator.TryValidateProperty(propertyValue, validationContext, results);
                    _messages.Clear(fieldIdentifier);
-                    _messages.Add(fieldIdentifier, results.Select(result => result.ErrorMessage!));
+                    foreach (var result in CollectionsMarshal.AsSpan(results))


Is this faster than foreach (var result in results) because it skips something like checking-whether-the-list-was-modified on each read?

If so, then as a meta-point about this feature itself rather than your use of it here, it seems like a weird way of expressing the intent because there's nothing obvious in the naming of CollectionsMarshal.AsSpan that implies this, or provides any indication of what would go wrong if the assumption turned out to be false. Perhaps this is something we only expect advanced .NET devs to do and don't feel it needs to be made any more obvious.

It's this "unsafe" way of getting you access to the internal array. The *Marshal is a pattern .NET uses to express that these APIs are advanced and could do bad things if misused

Yup - it basically gives you access to the static array that the list is using as it's implementation: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/CollectionsMarshal.cs#L19. It's only safe to use if the list isn't modified as part of the iteration which is true in this particular case.

src/Components/Forms/src/ValidationMessageStore.cs

SteveSandersonMS

Excellent stuff!

ghost · 2021-04-23T15:51:53Z

Hello @pranavkm!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

davidfowl · 2021-04-23T18:59:00Z

src/Components/WebAssembly/WebAssembly/src/Hosting/WebAssemblyHost.cs

@@ -158,12 +158,27 @@ internal async Task RunAsyncCore(CancellationToken cancellationToken, WebAssembl
                var loggerFactory = Services.GetRequiredService<ILoggerFactory>();
                _renderer = new WebAssemblyRenderer(Services, loggerFactory);

-                var rootComponents = _rootComponents;
-                foreach (var rootComponent in rootComponents)
+                var initializationTcs = new TaskCompletionSource();


I don't see a TaskCreationOptions.RunContinuationsAsynchronously

* Replace with alternatives or iteration where possible * Rewrite Router component to avoid allocations

pranavkm force-pushed the prkrishn/no-linq branch from 18066d1 to 6876339 Compare March 24, 2021 18:48

pranavkm marked this pull request as ready for review March 24, 2021 20:09

pranavkm requested a review from a team as a code owner March 24, 2021 20:09

eerhardt reviewed Mar 24, 2021

View reviewed changes

src/Components/Authorization/src/AttributeAuthorizeDataCache.cs Show resolved Hide resolved

eerhardt reviewed Mar 24, 2021

View reviewed changes

src/Components/Components/src/Reflection/ComponentProperties.cs Show resolved Hide resolved

Pilchie added the area-blazor Includes: Blazor, Razor Components label Mar 24, 2021

pranavkm force-pushed the prkrishn/no-linq branch from cce95ac to cf9a4f1 Compare March 25, 2021 02:13