Use factory pattern for caching in ControllerActionInvoker #6243

pranavkm · 2017-05-08T20:22:27Z

pranavkm · 2017-05-08T20:24:23Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/ControllerActivatorProvider.cs

+        private readonly Func<ControllerContext, object> _controllerActivatorCreate;
+        private readonly Action<ControllerContext, object> _controllerActivatorRelease;
+
+        public ControllerActivatorProvider(IEnumerable<IControllerActivator> activators)


Presumably we want to keep IControllerActivator \ IControllerFactory around. Thoughts on an alternate pattern to invoke them?

Yes, we does

Is your idea that we just wouldn't register the default implementations?

I actually removed the implementations. My plan was to leave the contract in to continue other systems that implemented it, but not carry forward our implementation. Alternatively, we can register the current one and check for an exact type match.

pranavkm · 2017-05-08T20:40:11Z

Before

After

pranavkm · 2017-05-08T20:41:10Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/ControllerActivatorProvider.cs

+
+            if (typeof(IDisposable).GetTypeInfo().IsAssignableFrom(descriptor.ControllerTypeInfo))
+            {
+                return _controllerActivatorRelease;


Needs to return _dispose

rynowak · 2017-05-08T21:03:32Z

@pranavkm that's not really enough data to be meaningful. You really need to get some significant CPU usage and profile for 30s - or collect some real world measurements with RPS.

rynowak · 2017-05-08T21:03:54Z

I still think we want to use this pattern even if it's a sideways move for perf, it's a cleaner design overall.

rynowak · 2017-05-08T21:06:35Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerActivator.cs

-    /// <summary>
-    /// <see cref="IControllerActivator"/> that uses type activation to create controllers.
-    /// </summary>
-    public class DefaultControllerActivator : IControllerActivator


This would be a breaking change for anyone that's subclassed it. Get approval from @Eilon

Yup. The alternative would be to leave this type in and registered, but use the factory if typeof(DefaultControllerActivator) == controllerActivator.GetType()

Where's a bug tracking this work?

If we delete DefaultControllerActivator, what would the alternative be to anyone who used/derived from it?

We spoke about it offline, but I don't think there's a tracking work item for it. I'll make for now.

I'll revive the types. We can do an exact match test so that it leaves users relying on the type unaffected.

Ok then that sounds reasonable to me.

rynowak · 2017-05-08T21:06:44Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerFactory.cs

-    /// <summary>
-    /// Default implementation for <see cref="IControllerFactory"/>.
-    /// </summary>
-    public class DefaultControllerFactory : IControllerFactory


rynowak · 2017-05-08T21:08:28Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCache.cs

@@ -43,14 +57,13 @@ private InnerCache CurrentCache
            }
        }

-        public ControllerActionInvokerState GetState(ControllerContext controllerContext)
+        public (ControllerActionInvokerState State, IFilterMetadata[] Filters) GetState(ControllerContext controllerContext)


We need to return two pieces of data (the cache entry and the filters. Any suggestions on how you'd want to surface this?

rynowak · 2017-05-08T21:08:53Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCache.cs

-            public ConcurrentDictionary<ActionDescriptor, Entry> Entries { get; } =
-                new ConcurrentDictionary<ActionDescriptor, Entry>();
+            public ConcurrentDictionary<ActionDescriptor, ControllerActionInvokerState> Entries { get; } =
+                new ConcurrentDictionary<ActionDescriptor, ControllerActionInvokerState>();


Can we call this FooCacheEntry?

rynowak · 2017-05-08T21:09:31Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerProvider.cs

            {
                var controllerContext = new ControllerContext(context.ActionContext);
                // PERF: These are rarely going to be changed, so let's go copy-on-write.
                controllerContext.ValueProviderFactories = new CopyOnWriteList<IValueProviderFactory>(_valueProviderFactories);
                controllerContext.ModelState.MaxAllowedErrors = _maxModelValidationErrors;

-                var cacheState = _controllerActionInvokerCache.GetState(controllerContext);
+                var cacheResult = _controllerActionInvokerCache.GetState(controllerContext);


cacheEntry

rynowak · 2017-05-08T21:09:46Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerState.cs

+            FilterItem[] filters,
+            Func<ControllerContext, object> controllerFactory,
+            Action<ControllerContext, object> controllerReleaser,
+            ControllerBinderFactoryProvider.BinderFactory propertyBinderFactory,


rynowak · 2017-05-08T21:10:35Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerState.cs

+
+        public ControllerBinderFactoryProvider.BinderFactory BinderFactory { get; }
+
+        public ObjectMethodExecutor ActionMethodExecutor { get; }


We might need to avoid making this public... this is a shared source type. It can create all kinds of havoc.

rynowak · 2017-05-08T21:11:14Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerBinderFactoryProvider.cs

+{
+    public static class ControllerBinderFactoryProvider
+    {
+        public delegate Task BinderFactory(


This is a terrible name. ControllerBinder? perhaps?

ControllerBinderDelegate?

ControllerBinderHelper 👍

rynowak · 2017-05-08T21:13:09Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/IControllerPropertyActivatorFactory.cs

 namespace Microsoft.AspNetCore.Mvc.Internal
 {
-    public interface IControllerPropertyActivator
+    public interface IControllerPropertyActivatorFactory


Provider rather than factory would be consistent with the other changes in this PR

rynowak · 2017-05-08T21:14:27Z

src/Microsoft.AspNetCore.Mvc.Core/ModelBinding/ParameterBinder.cs

+            IModelBinder modelBinder,
+            IValueProvider valueProvider,
+            ParameterDescriptor parameter,
+            ModelMetadata modelMetadata,


is modelMetadata allowed to be null? It's also called metadata elsewhere in this file, choose one

Eilon · 2017-05-09T17:13:21Z

src/Microsoft.AspNetCore.Mvc.Core/Controllers/DefaultControllerActivator.cs

-    /// <summary>
-    /// <see cref="IControllerActivator"/> that uses type activation to create controllers.
-    /// </summary>
-    public class DefaultControllerActivator : IControllerActivator


Where's a bug tracking this work?

If we delete DefaultControllerActivator, what would the alternative be to anyone who used/derived from it?

pranavkm · 2017-05-15T19:46:19Z

🆙 📅

pranavkm · 2017-05-17T19:29:19Z

BUMP

rynowak · 2017-05-19T15:03:58Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCacheEntry.cs

+            ControllerFactory = controllerFactory;
+            ControllerReleaser = controllerReleaser;
+            ControllerBinderDelegate = controllerBinderDelegate;
+            Filters = filters;


Can you call this cachedFilters or something to make it clear that it's not supposed to be used as-is?

rynowak · 2017-05-19T15:05:08Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/DefaultControllerPropertyActivator.cs


        public void Activate(ControllerContext context, object controller)
        {
+            EnsureInitialized();


Can we use LazyInitializer here instead?

rynowak · 2017-05-19T15:05:57Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/IControllerPropertyActivatorFactory.cs

@@ -1,10 +1,15 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

+using System;
+using Microsoft.AspNetCore.Mvc.Controllers;
+
 namespace Microsoft.AspNetCore.Mvc.Internal
 {
    public interface IControllerPropertyActivator
    {
        void Activate(ControllerContext context, object controller);


Is this still needed? Why/where?

We use it in DefaultControllerFactory (since that type wasn't changed).

OK I'm not super worried about this because it's internal any way.

rynowak · 2017-05-19T15:06:45Z

...ft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/ViewDataDictionaryControllerPropertyActivator.cs

        }

        public void Activate(ControllerContext actionContext, object controller)
        {
+            EnsureInitialized();


Use LazyInitializer

rynowak · 2017-05-19T15:07:18Z

This is looking good overall

pranavkm · 2017-05-19T23:00:44Z

🆙 📅

rynowak · 2017-05-22T16:16:03Z

src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvokerCache.cs

@@ -43,14 +57,13 @@ private InnerCache CurrentCache
            }
        }

-        public ControllerActionInvokerState GetState(ControllerContext controllerContext)
+        public (ControllerActionInvokerCacheEntry CacheEntry, IFilterMetadata[] Filters) GetCachedResult(ControllerContext controllerContext)


Use lowercase names with tuples

That's what the C# picked as the convention

pranavkm requested a review from rynowak May 8, 2017 20:22

dnfclas added the cla-already-signed label May 8, 2017

pranavkm commented May 8, 2017

View reviewed changes

rynowak reviewed May 8, 2017

View reviewed changes

Eilon suggested changes May 9, 2017

View reviewed changes

pranavkm force-pushed the prkrishn/controller-caching branch 2 times, most recently from 7ee0aa8 to 37f7e72 Compare May 15, 2017 19:40

rynowak reviewed May 19, 2017

View reviewed changes

Use factory pattern for caching in ControllerActionInvoker

0b1c44b

pranavkm force-pushed the prkrishn/controller-caching branch from 37f7e72 to 0baba4d Compare May 19, 2017 18:17

Changes per PR comments

267ea01

pranavkm force-pushed the prkrishn/controller-caching branch from 0baba4d to 267ea01 Compare May 19, 2017 22:37

rynowak reviewed May 22, 2017

View reviewed changes

rynowak approved these changes May 22, 2017

View reviewed changes

pranavkm closed this May 22, 2017

pranavkm deleted the prkrishn/controller-caching branch May 22, 2017 21:20


		public ControllerBinderFactoryProvider.BinderFactory BinderFactory { get; }

		public ObjectMethodExecutor ActionMethodExecutor { get; }

Use factory pattern for caching in ControllerActionInvoker #6243

Use factory pattern for caching in ControllerActionInvoker #6243

Conversation

pranavkm commented May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented May 8, 2017

Before

After

Choose a reason for hiding this comment

rynowak commented May 8, 2017

rynowak commented May 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented May 15, 2017

pranavkm commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented May 19, 2017

pranavkm commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented May 8, 2017 •

edited

Loading