PERF: Fix #6923 - Implement DbContext pooling for high scale scenarios. #6950

anpete · 2016-11-05T18:55:45Z

Initial impl. of context pooling.

anpete · 2016-11-05T18:56:22Z

ajcvickers · 2016-11-07T19:06:40Z

src/Microsoft.EntityFrameworkCore/Internal/DbContextPool.cs

+                return context;
+            }
+
+            context = ActivatorUtilities.CreateInstance<TContext>(serviceProvider);


This probably has the same issues as #6826

Is this a DI bug? I can't think of any other way to do this - We can't call GetService here.

I think it is a D.I. bug.

aspnet/DependencyInjection#197

OK, it looks like we should validate that the context type does not have a default ctor.

I wonder if this is a symptom of a wider issue. The DbContext class is being registered in D.I. as a scoped service, but it isn't really used that way because of the pool. So what happens if it depends on other scoped services? How does it get the correct instance of those services?

Two other architectures to consider:

Make the pool explicit such that other services (e.g. controllers) depend on the pool and grab a context from the pool when needed. The DbContext would never be registered in D.I. at all.

Be like DbConnection where the instance of DbContext itself is created by D,I, in the normal way, so it gets correctly injected other services, but the guts of DbContext is obtained from a pool.

We still want scoped here because we want still want context-per-request semantics. Its just that with pooling, a request defines the scope of exclusive ownership of a pooled instance, not the physical lifetime of the instance. Regarding "correct" instances, this is just the reset problem. Either, the scoped service is safe to re-use (because it is stateless and the scoped-ness is an artifact of our current architecture); or, we need a safe way of resetting the existing instance - like we do with the StateManager for example.

Could we leverage ActivactorUtilities.CreateFactory() here?

ajcvickers · 2016-11-07T19:08:56Z

src/Microsoft.EntityFrameworkCore/Internal/DbContextPool.cs

+            {
+                // Race to set this
+                _takenSnapshot = true;
+                _configurationSnapshot = ((IDbContextPoolable)context).SnapshotConfiguration();


Doesn't this need memory barrier protection?

Can you elaborate? The intent of this is we might end up calling Snapshot multiple times, but we don't care which one we end up with - they should all be equivalent.

How do we guarantee that the object has been fully constructed when viewed from another thread? Pretty sure this needs some form of memory barrier.

I see. Would changing the snapshot to be a class and removing the flag be a better option?

I don't see anyway to do it without a memory barrier.

Oh, so another thread could see _configurationSnapshot != null even though _configurationSnapshot was not fully constructed?

ajcvickers · 2016-11-07T19:38:04Z

Clean up transaction state? Reset command timeout?

anpete · 2016-11-07T19:48:04Z

@ajcvickers Thanks, was sure I had missed stuff. With transactions, is it just clearing IRelationalConnection.CurrentTransaction?

ajcvickers · 2016-11-07T20:08:10Z

Not sure about the transactions. @AndriySvyryd would know.

AndriySvyryd · 2016-11-07T20:26:09Z

Calling IRelationalConnection.Dispose should be enough to reset the transaction state

AndriySvyryd · 2016-11-07T20:35:21Z

I think IDbContextPoolable should provide a way to subscribe to the resetting, so non-core services are able to participate

anpete · 2016-11-07T22:09:43Z

@AndriySvyryd It would be great to see an example of where this is needed to understand what the solution might look like.

AndriySvyryd · 2016-11-07T22:11:14Z

@anpete The IRelationalConnection above for instance.

anpete · 2016-11-07T22:14:10Z

@AndriySvyryd Great! Will take a look. BTW, it looks like it is safe to call RelationalConnection.Dispose but then keep using it?! Is this by design?

AndriySvyryd · 2016-11-07T22:16:16Z

@anpete I believe it's because there was no real downside to this approach and it was less code

anpete · 2016-11-08T20:25:27Z

@AndriySvyryd Added provider services reset in latest commit.

AndriySvyryd · 2016-11-08T22:10:00Z

src/Microsoft.EntityFrameworkCore/EntityFrameworkServiceCollectionExtensions.cs

+        /// <param name="serviceCollection"> The <see cref="IServiceCollection" /> to add services to. </param>
+        /// <param name="optionsAction">
+        ///     <para>
+        ///         An required action to configure the <see cref="DbContextOptions" /> for the context. When using


Typo in "An"

AndriySvyryd · 2016-11-08T22:42:20Z

src/Microsoft.EntityFrameworkCore/EntityFrameworkServiceCollectionExtensions.cs

+        ///         <see cref="IServiceProvider" />
+        ///         for internal Entity Framework services.
+        ///     </para>
+        /// </summary>


We should mention in the summary for both overloads that polling would be enabled so no state should be maintained on the context and OnConfiguring will not be called

AndriySvyryd · 2016-11-08T22:51:23Z

src/Microsoft.EntityFrameworkCore/Internal/DbContextPool.cs

+    public class DbContextPool<TContext> : IDbContextPool, IDisposable
+        where TContext : DbContext
+    {
+        private const int DefaultPoolSize = 32;


Different hardware configurations need different values, so it would be more useful if this was based on ThreadPool.GetMinThreads, though with current API it would be rarely used.

AndriySvyryd · 2016-11-08T22:57:25Z

contingent on @ajcvickers signoff

ajcvickers · 2016-11-08T23:08:36Z

I'm still not happy with the use of scoped services like this. Feels like pit of failure to me since you have to understand the scoped-per-request-but-not-really semantics. If @divega is fine with it, then he's the boss. To me it feels like there are better architectural options.

divega · 2016-11-08T23:30:13Z

I would prefer to sort out @ajcvickers' concerns in person. I would also like to see if we can use ActivatorUtilities.CreateFactory() to avoid some reflection/expression compilation overhead each time we actually do have to create a new context instance.

divega · 2016-11-08T23:44:30Z

src/Microsoft.EntityFrameworkCore/EntityFrameworkServiceCollectionExtensions.cs

+        /// <returns>
+        ///     The same service collection so that multiple calls can be chained.
+        /// </returns>
+        public static IServiceCollection AddDbContext<TContext>(


Another thing to chat about: Is it better to add these new overloads of AddDbContext<TContext>() that take poolSize or instead have a new method, e.g. AddDbContextPool<TContext>() that makes it a bit clearer that you are forking from the regular behavior of AddDbContext, for which poolSize is optional (or not specified in all overloads) so that we can pick a good default for you, and for which we can also have very specific and separate documentation/xml comments.

davidroth · 2016-11-10T13:16:36Z

@anpete Out of curiosity: Can you share some benchmarks about this improvement?

anpete · 2016-11-16T01:02:03Z

@ajcvickers @AndriySvyryd @divega Added a commit which I think addresses the decisions from today. Let me know if I missed something.

anpete · 2016-11-16T05:32:20Z

src/Microsoft.EntityFrameworkCore/Internal/DbContextPool.cs

+                if (parameters.Length == 1
+                    && (parameters[0].ParameterType == typeof(DbContextOptions) 
+                        || parameters[0].ParameterType == typeof(DbContextOptions<TContext>)))
+                {


@ajcvickers Spoke to @divega about allowing non-generic options here and he felt it was safe. Any concerns?

@anpete @divega Non-generic options is fine. Hopefully this makes it work with non-derived DbContext. Maybe add a test for that?

@ajcvickers Added the test. Had to loosen the ctor validation logic slightly to ignore NonPublic ctors.

divega · 2016-11-16T05:49:31Z

src/Microsoft.EntityFrameworkCore/EntityFrameworkServiceCollectionExtensions.cs

+            [NotNull] this IServiceCollection serviceCollection,
+            [NotNull] Action<IServiceProvider, DbContextOptionsBuilder> optionsAction,
+            int poolSize = 128)
+            where TContext : DbContext


Assuming we can and it makes sense to use the API that @AndriySvyryd had suggested for an optimal pool size, I think we should consider making it of type int? or using overloads instead of an optional parameters.

The rest LGTM.

@divega ThreadPool.GetMinThreads is not what we want here, at least not for web apps. For a web app, what we really want is something close to the average number of concurrent requests (FYI IIS defaults to 5000 for maxConcurrentRequests for example). So I think we may just need to pick a number that is somewhat sensible.

ajcvickers

divega

- Initial impl. of context pooling.

…providers.

1) Creates separate registration methods for pooling: AddDbContextPool 2) Runs OnConfiguring, throws if the options were modified. 3) Adds context ctor validation. 4) No longer uses DI for context instantiation.

dnfclas added the cla-already-signed label Nov 5, 2016

anpete added the needs-review label Nov 5, 2016

ajcvickers reviewed Nov 7, 2016

View reviewed changes

anpete force-pushed the fix-6923 branch 3 times, most recently from 6d1ac97 to 7e32823 Compare November 8, 2016 20:24

AndriySvyryd reviewed Nov 8, 2016

View reviewed changes

divega reviewed Nov 8, 2016

View reviewed changes

anpete force-pushed the fix-6923 branch from 7e32823 to 010a1e1 Compare November 9, 2016 18:38

smitpatel changed the base branch from feature/1.2.0 to dev November 10, 2016 18:57

anpete force-pushed the fix-6923 branch 2 times, most recently from 11de381 to 0b1bfd1 Compare November 16, 2016 00:59

anpete commented Nov 16, 2016

View reviewed changes

divega reviewed Nov 16, 2016

View reviewed changes

ajcvickers approved these changes Nov 16, 2016

View reviewed changes

ajcvickers removed the needs-review label Nov 16, 2016

anpete force-pushed the fix-6923 branch 3 times, most recently from 8fc217b to 8696cb8 Compare November 16, 2016 18:40

divega approved these changes Nov 16, 2016

View reviewed changes

anpete force-pushed the fix-6923 branch 2 times, most recently from 2f10fec to 40dd5b0 Compare November 16, 2016 19:24

anpete added 4 commits November 16, 2016 13:09

PERF: Fix #6923 - Implement DbContext pooling for high scale scenarios.

4376b2d

- Initial impl. of context pooling.

Switch to NonCapturingLazyInitializer for snapshot initialization.

ddbbeb3

Added provider services reset. Transactions are reset for relational …

2c0cbed

…providers.

DbContext pooling design review feedback:

b1e287a

1) Creates separate registration methods for pooling: AddDbContextPool 2) Runs OnConfiguring, throws if the options were modified. 3) Adds context ctor validation. 4) No longer uses DI for context instantiation.

anpete force-pushed the fix-6923 branch from 40dd5b0 to b1e287a Compare November 16, 2016 21:09

anpete merged commit b1e287a into dev Nov 16, 2016

anpete deleted the fix-6923 branch November 16, 2016 21:41

dotnetjunkie mentioned this pull request Oct 20, 2017

What's the performance benefit of pooling DbContexts #10125

Closed

PERF: Fix #6923 - Implement DbContext pooling for high scale scenarios. #6950

PERF: Fix #6923 - Implement DbContext pooling for high scale scenarios. #6950

Conversation

anpete commented Nov 5, 2016

anpete commented Nov 5, 2016

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

divega Nov 8, 2016 • 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

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

ajcvickers commented Nov 7, 2016

anpete commented Nov 7, 2016

ajcvickers commented Nov 7, 2016

AndriySvyryd commented Nov 7, 2016

AndriySvyryd commented Nov 7, 2016

anpete commented Nov 7, 2016

AndriySvyryd commented Nov 7, 2016

anpete commented Nov 7, 2016

AndriySvyryd commented Nov 7, 2016

anpete commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndriySvyryd commented Nov 8, 2016

ajcvickers commented Nov 8, 2016

divega commented Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

davidroth commented Nov 10, 2016 • edited Loading

anpete commented Nov 16, 2016

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

ajcvickers left a comment

Choose a reason for hiding this comment

divega left a comment

Choose a reason for hiding this comment

divega Nov 8, 2016 •

edited

Loading

divega commented Nov 8, 2016 •

edited

Loading

davidroth commented Nov 10, 2016 •

edited

Loading