Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change timer from method variable to property of SqlConnectioninteral #26495

Closed
wants to merge 17 commits into from
Closed

Change timer from method variable to property of SqlConnectioninteral #26495

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d03467f
Host timer as property on SqliteConnectionInternal.
Russell-Horwood-Xceptor Oct 31, 2021
a5b7dee
Merge branch 'dotnet:main' into 26295_SqliteTimerAsField
Nov 1, 2021
97fbfed
Summary of the changes
Russell-Horwood-Xceptor Oct 31, 2021
2c0520a
Merge branch '26295_SqliteTimerAsField' of https://github.com/Dr-Ogde…
Russell-Horwood-Xceptor Nov 1, 2021
0581078
Update src/Microsoft.Data.Sqlite.Core/SqliteConnectionInternal.cs
Nov 1, 2021
af8f49f
Summary of the changes
Russell-Horwood-Xceptor Nov 1, 2021
e9ba6d3
Merge from remote.
Russell-Horwood-Xceptor Nov 1, 2021
1abe9e8
Summary of the changes
Russell-Horwood-Xceptor Nov 1, 2021
2431c1d
Merge branch 'main' into 26295_SqliteTimerAsField
Nov 19, 2021
827b12e
Fix my merge-by-eye fail.
Russell-Horwood-Xceptor Nov 19, 2021
44ca0ca
Fix my merge-by-eye fail.
Russell-Horwood-Xceptor Nov 19, 2021
08a8b2a
Merge branch '26295_SqliteTimerAsField' of https://github.com/Dr-Ogde…
Russell-Horwood-Xceptor Nov 19, 2021
f5649e9
Merge branch '26295_SqliteTimerAsField' of https://github.com/Dr-Ogde…
Russell-Horwood-Xceptor Nov 19, 2021
7d53c2d
Merge branch '26295_SqliteTimerAsField' of https://github.com/Dr-Ogde…
Russell-Horwood-Xceptor Nov 20, 2021
dfd9adc
Merge branch 'main' into 26295_SqliteTimerAsField
Russell-Horwood-Xceptor Feb 6, 2022
685a862
Instantiate timer iff the command is the only one on the connection
Russell-Horwood-Xceptor Feb 6, 2022
f2ef784
Same timer prepare and reading statements
Russell-Horwood-Xceptor Feb 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions src/Microsoft.Data.Sqlite.Core/SqliteCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
using System.Collections.Generic;
using System.Data;
using System.Data.Common;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Text;
Expand Down Expand Up @@ -256,9 +255,7 @@ public override void Prepare()
return;
}

var timer = new Stopwatch();

using var enumerator = PrepareAndEnumerateStatements(timer).GetEnumerator();
using var enumerator = PrepareAndEnumerateStatements().GetEnumerator();
while (enumerator.MoveNext())
{
}
Expand Down Expand Up @@ -307,19 +304,19 @@ public override void Prepare()
throw new InvalidOperationException(Resources.TransactionCompleted);
}

var timer = new Stopwatch();
var closeConnection = behavior.HasFlag(CommandBehavior.CloseConnection);

var dataReader = new SqliteDataReader(this, timer, GetStatements(timer), closeConnection);
_connection.Timer.Reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right to me. A connection can have multiple readers open at the same time. I'm pretty sure we need a new instance for every reader...

Copy link
Contributor

Choose a reason for hiding this comment

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

Would using something like ValueStopwatch instead also help perf?

Copy link
Member

@roji roji Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

See dotnet/runtime#48570 for some discussion on ValueStopWatch (which was closed for some reason).

If we don't do ValueStopWatch, we can still "pool" a single StopWatch instance on the connection, so that uses which don't involve multiple open readers can benefit (but those with multiple readers would allocate).

Copy link
Contributor

@vonzshik vonzshik Jan 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Huh, didn't know Sqlite supported MARS...

To me, using ValueStopwatch seems to be the simplest solution (in a sense that there is no pooling involved, which does complicate things a bit), but pooling is also fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, ValueStopwatch in a way it's implemented in ASP.NET Core wouldn't work here. The main offender is SqliteDataReader:

efcore/src/Microsoft.Data.Sqlite.Core/SqliteDataReader.cs

Lines 157 to 173 in afe299f

_timer.Start();
while (IsBusy(rc = sqlite3_step(stmt)))
{
if (_command.CommandTimeout != 0
&& _timer.ElapsedMilliseconds >= _command.CommandTimeout * 1000L)
{
break;
}
sqlite3_reset(stmt);
// TODO: Consider having an async path that uses Task.Delay()
Thread.Sleep(150);
}
_timer.Stop();

Every time a NextResult is called on a reader the timer isn't reset, but continues to measure the elapsed time from where it left on a previous call.

Copy link
Author

Choose a reason for hiding this comment

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

If we don't do ValueStopWatch, we can still "pool" a single StopWatch instance on the connection, so that uses which don't involve multiple open readers can benefit (but those with multiple readers would allocate).

@roji Could this be something as simple as allocating a new Stopwatch if the connection has multiple commands like my latest change?

Every time a NextResult is called on a reader the timer isn't reset, but continues to measure the elapsed time from where it left on a previous call.

@vonzshik My initial feeling is that adding stop/start functionality to ValueStopwatch would result in something almost the same as the standard Stopwatch implementation, so not much benefit. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

@vonzshik My initial feeling is that adding stop/start functionality to ValueStopwatch would result in something almost the same as the standard Stopwatch implementation, so not much benefit. What do you think?

Well, there is the fun fact that both SqliteCommand.PrepareAndEnumerateStatements and SqliteDataReader.NextResult are supposed to use the exact same instance of Stopwatch and the result from SqliteCommand.PrepareAndEnumerateStatements is passed as IEnumerable to the reader...

efcore/src/Microsoft.Data.Sqlite.Core/SqliteCommand.cs

Line 313 in 802d4c5

var dataReader = new SqliteDataReader(this, timer, GetStatements(timer), closeConnection);

Copy link
Author

Choose a reason for hiding this comment

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

Hey @vonzshik, that is a fun fact.
Sorry, I've been so slow getting to this, and for being a total noob on this project, but would you mind taking another look, please.

var dataReader = new SqliteDataReader(this, _connection.Timer, GetStatements(), closeConnection);
dataReader.NextResult();

return DataReader = dataReader;
}

private IEnumerable<sqlite3_stmt> GetStatements(Stopwatch timer)
private IEnumerable<sqlite3_stmt> GetStatements()
{
foreach (var stmt in !_prepared
? PrepareAndEnumerateStatements(timer)
? PrepareAndEnumerateStatements()
: _preparedStatements)
{
var boundParams = _parameters?.Bind(stmt) ?? 0;
Expand Down Expand Up @@ -467,7 +464,7 @@ public override void Cancel()
{
}

private IEnumerable<sqlite3_stmt> PrepareAndEnumerateStatements(Stopwatch timer)
private IEnumerable<sqlite3_stmt> PrepareAndEnumerateStatements()
{
DisposePreparedStatements(disposing: false);

Expand All @@ -478,23 +475,24 @@ private IEnumerable<sqlite3_stmt> PrepareAndEnumerateStatements(Stopwatch timer)
int rc;
sqlite3_stmt stmt;
var start = 0;
_connection!.Timer.Reset();
do
{
timer.Start();
_connection.Timer.Start();

ReadOnlySpan<byte> tail;
while (IsBusy(rc = sqlite3_prepare_v2(_connection!.Handle, sql.AsSpan(start), out stmt, out tail)))
while (IsBusy(rc = sqlite3_prepare_v2(_connection.Handle, sql.AsSpan(start), out stmt, out tail)))
{
if (CommandTimeout != 0
&& timer.ElapsedMilliseconds >= CommandTimeout * 1000L)
&& _connection.Timer.ElapsedMilliseconds >= CommandTimeout * 1000L)
{
break;
}

Thread.Sleep(150);
}

timer.Stop();
_connection.Timer.Stop();
start = sql.Length - tail.Length;

SqliteException.ThrowExceptionForRC(rc, _connection.Handle);
Expand Down
9 changes: 9 additions & 0 deletions src/Microsoft.Data.Sqlite.Core/SqliteConnection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,15 @@ public override string ConnectionString
internal SqliteConnectionStringBuilder ConnectionOptions
=> PoolGroup.ConnectionOptions;

internal Stopwatch Timer
{
get
{
Debug.Assert(_innerConnection != null);
return _innerConnection!.Timer;
}
}

/// <summary>
/// Gets the name of the current database. Always 'main'.
/// </summary>
Expand Down
2 changes: 2 additions & 0 deletions src/Microsoft.Data.Sqlite.Core/SqliteConnectionInternal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,8 @@ public bool CanBePooled
public sqlite3? Handle
=> _db;

public Stopwatch Timer { get; } = new();

public void DoNotPool()
=> _canBePooled = false;

Expand Down