SqliteCommand allocates one Stopwatch per every query or prepare

[vonzshik]

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

Line 259 in 9b66f4c

var timer = new Stopwatch();

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

Line 310 in 9b66f4c

var timer = new Stopwatch();

They can be allocated once per SqliteConnection.

[ghost]

@vonzshik "They can be allocated once per SqliteConnection."

So, is this simply a case of declaring the 'timer' Stopwatch as a field instead of a method variable?

[vonzshik]

@Dr-Ogden-Wernstrom more like an internal property on SqliteConnection which returns a Stopwatch instance, something like:

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

The main question here is, should the instance of Stopwatch be kept on SqliteConnection or on SqliteConnectionInternal (this way we do pay a little bit every time we're going to access Timer, but on the other hand that's going to reduce SqliteConnection allocations).

[ghost]

Thanks @vonzshik, understood. I've never contributed before but would like to try these options out, see what works best, and take a crack at a PR.

[ghost]

So I think it makes more sense to hold the StopWatch on SqliteConnection rather than SqlConnectionInternal because an internal connection is created each time the connection is opened (and destroyed each time the connection is closed). The same stopwatch can be re-used by all instances of SqliteConnectionInternal that SqliteConnection creates.

[vonzshik]

@Dr-Ogden-Wernstrom not unless pooling is enabled (which it is by default).

[ghost]

@vonzshik Ok (can't believe I missed that). So makes more sense to put on the internal connection (internal getter with initializer) and put another internal property on SqliteConnection that simply passes through the timer from the internal connection, or returns null if called when the connection has not yet been opened.

The only downside is when pooling is disabled then an allocation is made each time the connection is opened, however since this is not the default setting and still this is better than the current code, so this is acceptable?

[vonzshik]

@Dr-Ogden-Wernstrom pooling is a very new feature (it should be released together with 6.0 release), so not a lot of people actually know about that)

So makes more sense to put on the internal connection (internal getter with initializer) and put another internal property on SqliteConnection that simply passes through the timer from the internal connection, or returns null if called when the connection has not yet been opened.

Pretty much, yes. Although, I'm not sure SqliteConnection.Timer supposed to ever return a null, since you have to have an internal connection to execute a query, so in the end I think it should look something like this:

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

[ghost]

@vonzshik That it never returns null I did spot. However, I reasoned that the property itself doesn't know that it's always called when it has a value. Only the calling code knows that. So I thought maybe more appropriate to define the property on SqliteConnection as

internal Stopwatch? Timer => _innerConnection?.Timer;

and use null-forgiving from SqliteCommand.PrepareAndEnumerateStatements

 _connection!.Timer!.Reset();
do
{
    _connection.Timer.Start();

    ReadOnlySpan<byte> tail;

    // etc.

I've pushed a commit with this in so you can see more fully: https://github.com/Dr-Ogden-Wernstrom/efcore/commit/d03467ff09b0326ee33e718fa9bb9c725549258c

That's what I would do if it were my project anyway. But I'm happy to follow your lead on this one.

[vonzshik]

@Dr-Ogden-Wernstrom but you could just do the same at the SqliteConnection.Timer property, so you don't have to put a ! everywhere.

Anyway, I think the best way to go forward is to submit a pr, where we will discuss further.

[3schwartz]

Hi @vonzshik and @Dr-Ogden-Wernstrom

This hasn't been touch for a while - would it be ok I looked into it?

[ajcvickers]

@3schwartz Note that the previous PR was closed--see #26495. Probably only worth looking into if you have some other way of doing it that won't hit the same issues.

[3schwartz]

@3schwartz Note that the previous PR was closed--see #26495. Probably only worth looking into if you have some other way of doing it that won't hit the same issues.

I wasn't aware if that. I will find another first good issue then.

[ajcvickers]

Note from triage: does not seem to be a good way of doing this.