Microsoft.Data.Sqlite: Pool connections #25018

bricelam · 2021-06-01T22:56:26Z

Resolves #13837, fixes #25101, fixes #25078

TechEmpower Fortunes benchmark results on aspnet-citrine-win:

	5.0	6.0
Requests/sec	3,473	25,510
Mean latency (ms)	73.56	10.04
CPU Usage (%)	93	99

/cc @sebastienros

src/Microsoft.Data.Sqlite.Core/SqliteConnectionPool.cs

davidfowl · 2021-06-02T17:38:22Z

cc @sebastienros

src/Microsoft.Data.Sqlite.Core/SqliteConnection.cs

bricelam · 2021-06-04T23:10:25Z

src/Microsoft.Data.Sqlite.Core/SqliteConnection.cs


            _state = ConnectionState.Closed;
            OnStateChange(new StateChangeEventArgs(ConnectionState.Open, ConnectionState.Closed));
        }

+        internal void Deactivate()


We should consider resetting other connection state too. The EF Core tests were leaking PRAGMA foreign_keys = 0.

See #13837 (comment)

Yes, that seems important - state leaks via the pool are dangerous.

Perf-wise, in Npgsql, when a connection is returned to the pool, the SQL to reset its state is added to its outgoing buffer, but not executed (that would be an extra network roundtrip). Instead, when the connection is next rented out, the reset SQL will be prepended to the user's SQL in a batch.

I don't know what the overhead of a "roundtrip" is in Sqlite (certainly less important than a remote database server 🤣), but this could still well be worth doing. It may require a bit of bookkeeping but is otherwise pretty easy.

roji · 2021-06-15T11:28:27Z

src/EFCore.Sqlite.Core/Storage/Internal/SqliteRelationalConnection.cs

+            var connectionStringBuilder = new SqliteConnectionStringBuilder(GetValidatedConnectionString())
+            {
+                Mode = SqliteOpenMode.ReadOnly,
+                Pooling = false


Should be non-pooling? I get that it's used by Exists which probably shouldn't pool, but this is also a public method for creating read-only connections...

Meh, it's internal. Probably should have been inlined in the first place...

src/Microsoft.Data.Sqlite.Core/SqliteConnectionFactory.cs

roji · 2021-06-15T11:46:30Z

src/Microsoft.Data.Sqlite.Core/SqliteConnection.cs


            _state = ConnectionState.Closed;
            OnStateChange(new StateChangeEventArgs(ConnectionState.Open, ConnectionState.Closed));
        }

+        internal void Deactivate()


Yes, that seems important - state leaks via the pool are dangerous.

Perf-wise, in Npgsql, when a connection is returned to the pool, the SQL to reset its state is added to its outgoing buffer, but not executed (that would be an extra network roundtrip). Instead, when the connection is next rented out, the reset SQL will be prepended to the user's SQL in a batch.

I don't know what the overhead of a "roundtrip" is in Sqlite (certainly less important than a remote database server 🤣), but this could still well be worth doing. It may require a bit of bookkeeping but is otherwise pretty easy.

src/Microsoft.Data.Sqlite.Core/SqliteConnection.cs

roji · 2021-06-15T12:05:49Z

src/Microsoft.Data.Sqlite.Core/SqliteConnection.cs

+
+            int rc;
+
+            if (_collations != null)


This blocks removes the collations/functions/aggregates right?

If so, it may be worth considering allowing these to persist across pooled open/close... I don't know what the overhead is of continuously tearing these down and creating them again, but in a short-lived connection scenario (e.g. web) it may be a lot.

For comparison, in Npgsql prepared statements persist by default (not destroyed when the connection is returned to the pool), which is really necessary for good perf.

Could be handled later.

I'm mostly worried about memory leaks here--the delegates would hang around a lot longer than the connection you added them to if we held onto them in the pool.

Yeah, that's similar to how prepared statements work (but with server memory resources). At least for prepared statements, it seems pretty clear that an application would typically execute the same SQL across pooled connections, and forcing it to re-prepare every time after renting a connection would negate a lot of the perf gains.

I guess the main question here - what's the overhead of tearing these down and recreating them for each pooled connection? If it's a very light operation it indeed doesn't matter, but otherwise it could be important. Could also be an opt-in.

I assume (possibly a bad idea) that it's pretty cheap--it's just adding and removing function pointers to a dictionary.

src/Microsoft.Data.Sqlite.Core/SqliteConnection.cs

src/Microsoft.Data.Sqlite.Core/SqliteConnectionFactory.cs

roji · 2021-06-15T13:55:07Z

src/Microsoft.Data.Sqlite.Core/SqliteConnectionPoolGroup.cs

+            {
+                lock (this)
+                {
+                    if (_pool == null


If I got the pruning logic right, the first time prune happens, if the pool is empty (e.g. because its connections were pruned), its state is set to Idle. If it stays that way for the next prune, it becomes disabled, and at this point we'd return null from GetPool.

So basically if I do a single operation on a pool, and then go do something else for a long time, the pool will eventually become Disabled, right? At that point if I open a connection for the connection string, it won't be pooled?

If you open a connection for a pool group that's been disabled, it will re-activate. Disabled pool groups will eventually be freed and opening a new connection for it will re-create the group.

roji · 2021-06-15T13:56:38Z

src/Microsoft.Data.Sqlite.Core/SqliteConnectionPoolGroup.cs

+
+namespace Microsoft.Data.Sqlite
+{
+    internal class SqliteConnectionPoolGroup


I'm not completely clear on why this type is needed in addition to NpgsqlConnectionPool itself. Wouldn't it be possible to fold this functionality into that?

I gave it a try before submitting this PR but found that this factoring actually encapsulates the handling of pooled vs non-pooled connections pretty well.

roji · 2021-06-15T13:58:30Z

src/Microsoft.Data.Sqlite.Core/Utilities/MemberNotNullAttribute.cs

+// 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.
+
+#if !NET


Nowadays I think it's cooler to do NET5_OR_GREATER

barronpm · 2021-06-18T14:00:05Z

Resolves #13837

TechEmpower Fortunes benchmark results on aspnet-citrine-win:
5.0 6.0
Requests/sec 3,473 25,510
Mean latency (ms) 73.56 10.04
CPU Usage (%) 93 99

/cc @sebastienros

Have you made any performance comparisons between this branch and main? IMO that's a better target to see the benefit of this PR.

bricelam · 2021-06-18T22:50:59Z

@barronpm main was about 3,700 requests/second

bricelam · 2021-08-10T18:46:09Z

Unfortunately, I'm seeing ericsink/SQLitePCL.raw#430 locally a lot more frequently after this change...

roji · 2021-08-11T10:23:58Z

Just an idea - we can always make pooling opt-in via a switch (and call it experimental)... Would be sad to not merge this because of that bug (which can also occur without pooling, it seems)

bricelam · 2021-08-11T22:50:06Z

I really like the idea of defaulting the Pooling keyword to False for this release. That'll enable users to try it out and give us feedback before turning it on by default for everyone.

roji

Forgot to squirrel this

roji · 2021-08-11T22:52:32Z

I also like that this feature may help track down the actual bug because it makes it happen more often...

Resolves dotnet#13837

sebastienros · 2021-10-07T18:31:06Z

OrchardCore, Citrine Linux:
Before: 5.5K RPS
After: 14.5K RPS

bricelam requested a review from a team June 1, 2021 22:56

martincostello reviewed Jun 2, 2021

View reviewed changes

src/Microsoft.Data.Sqlite.Core/SqliteConnectionPool.cs Show resolved Hide resolved

bricelam commented Jun 4, 2021

View reviewed changes

src/Microsoft.Data.Sqlite.Core/SqliteConnection.cs Outdated Show resolved Hide resolved

bricelam commented Jun 4, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

bricelam force-pushed the pool4 branch from 55295f5 to 198f6f1 Compare June 10, 2021 20:26

AndriySvyryd requested a review from roji June 11, 2021 18:00

roji reviewed Jun 15, 2021

View reviewed changes

bricelam force-pushed the pool4 branch from 198f6f1 to ea3a867 Compare June 18, 2021 23:03

bricelam force-pushed the pool4 branch from ea3a867 to 8d6f3ce Compare August 9, 2021 17:47

bricelam mentioned this pull request Aug 10, 2021

Unexpected System.AccessViolationException when using Microsoft.Data.Sqlite ericsink/SQLitePCL.raw#430

Closed

bricelam force-pushed the pool4 branch from 8d6f3ce to 3a0b414 Compare August 11, 2021 22:46

roji approved these changes Aug 11, 2021

View reviewed changes

Microsoft.Data.Sqlite: Pool connections

9e2c1d9

Resolves dotnet#13837

bricelam force-pushed the pool4 branch from 3a0b414 to 9e2c1d9 Compare August 12, 2021 14:52

bricelam merged commit 9e2c1d9 into dotnet:main Aug 12, 2021

bricelam deleted the pool4 branch August 12, 2021 14:52

ajcvickers mentioned this pull request Aug 26, 2021

Entity Framework Biweekly Status Updates (2021) #23884

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Microsoft.Data.Sqlite: Pool connections #25018

Microsoft.Data.Sqlite: Pool connections #25018

bricelam commented Jun 1, 2021 •

edited

Loading

davidfowl commented Jun 2, 2021

bricelam Jun 4, 2021

bricelam Jun 4, 2021

roji Jun 15, 2021

This comment has been minimized.

roji Jun 15, 2021

bricelam Jun 18, 2021

roji Jun 15, 2021

roji Jun 15, 2021

bricelam Jun 17, 2021

roji Jun 17, 2021

bricelam Aug 9, 2021

roji Jun 15, 2021

bricelam Jun 17, 2021

roji Jun 15, 2021

bricelam Jun 17, 2021 •

edited

Loading

roji Jun 15, 2021

barronpm commented Jun 18, 2021

bricelam commented Jun 18, 2021

bricelam commented Aug 10, 2021 •

edited

Loading

roji commented Aug 11, 2021

bricelam commented Aug 11, 2021

roji left a comment

roji commented Aug 11, 2021

sebastienros commented Oct 7, 2021

Microsoft.Data.Sqlite: Pool connections #25018

Microsoft.Data.Sqlite: Pool connections #25018

Conversation

bricelam commented Jun 1, 2021 • edited Loading

davidfowl commented Jun 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

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

bricelam Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barronpm commented Jun 18, 2021

bricelam commented Jun 18, 2021

bricelam commented Aug 10, 2021 • edited Loading

roji commented Aug 11, 2021

bricelam commented Aug 11, 2021

roji left a comment

Choose a reason for hiding this comment

roji commented Aug 11, 2021

sebastienros commented Oct 7, 2021

bricelam commented Jun 1, 2021 •

edited

Loading

bricelam Jun 17, 2021 •

edited

Loading

bricelam commented Aug 10, 2021 •

edited

Loading