[Enhancement] Add Command Caching

[gentledepp]

I separated this enhancement from these issues: https://github.com/Mimetis/Dotmim.Sync/issues/255 and originally: https://github.com/Mimetis/Dotmim.Sync/issues/155

Have a look at this closed issue here:

Implementing command caching is not a lot of effort as your architecture makes it quite easy. I did some measurements and added them to an issue, but I can't seem to find it. So you have to believe me. Jedi-mode: on: You are convinced, that adding command caching is an absolute necessity!

[Mimetis]

I can imagine that it could be helpful for SQLite, but is it worth it for SQL Server and MySQL ?

[gentledepp]

yes for sure. Because with the cached command, MSSQL for example does not have to parse the SQL (less memory usage and thus GC pressure) and I guess (but don't know for sure) it also helps MSSQL cache the execution plan.

But it will have less of an impact on MSSQL if bulk-updates are used because you use much less SQL Commands (in MySQL and SQLite one command per row)

[Mimetis]

Do you have any pointer on a doc / blog post about the subject ?

[gentledepp]

Only my performance tests and [this stack overflow thread] (https://stackoverflow.com/questions/5818619/what-does-sqlcommand-prepare-do-and-when-should-it-be-used#:~:text=The%20Prepare%20method%20is%20actually,from%20it%20will%20pick%20up.&text=However%2C%20if%20you%20are%20not,optimized%20version%20of%20the%20query.)

Fact is:

• it is not hard to implement
• it can bring huge performance enhancements
• in cases it does not it does no harm

[Mimetis]

Jedi-mode: on: Wow, seems to ge a good idea to add command caching!

Hey @gentledepp, I've implemented the command caching. Not sure it's worth it, but a little voice in my head .. well.. you know...

Anyway I've added a new class DbCommandPool which will ensure the command caching and is thread safe

I had to remove all the command disposing and re-affect all the connections / transactions.

So far, if we look at the last pipeline tests, we can see that we have an increase of the performances:

Provider	Without Command caching	With Command caching	Diff	% Perf increase
SqlServerSyncProvider	25m 11s	23m28s	103 s	6.8 %
MySqlSyncProvider	29m07s	24m25s	282 s	16.1 %

Here are the tests results:

Without cache : Run 01 With cache : Run 02

As expected the MySql perf increase is much higher since we are calling much more the proc stocks than on the Sql Server tests

The only thing I did not measure is the memory increase between the 2 solutions. Hopefully not too high.

Let me know what you think

And Thanks for your precious help ! :)

Seb

[gentledepp]

@Mimetis - this static command cache is not a good idea. DbCommands are not thread safe. My original solution worked, as it cached the commands per request in the SQLiteSyncAdapter So commands are re-used per request, which already does a lot of good things.

Can you link your commit?

I am asking because implementing command caching can be easily done wrong. Take a look at my original implementation:

        public override DbCommand GetCommand(DbCommandType commandType, IEnumerable<string> additionals = null)
        {
            var key = new CommandCacheKey(commandType, additionals);
            if (!commandCache.TryGetValue(key, out var command))
            {
                command = this.Connection.CreateCommand();
                string text;

                if (additionals != null)
                    text = this.sqliteObjectNames.GetCommandName(commandType, additionals);
                else
                    text = this.sqliteObjectNames.GetCommandName(commandType);

                // on Sqlite, everything is text :)
                command.CommandType = CommandType.Text;
                command.CommandText = text;
                command.Connection = Connection;

                // ⚠ In case you set the transaction of a prepared command, the command is invalidated and the cache lost - EVEN IF IT IS THE SAME TRANSACTION INSTANCE!
                if (Transaction != null)
                    command.Transaction = Transaction;

                // add command so it can be reused
                commandCache.Add(key, command);
            }

            return command;
        }

        public override void SetCommandParameters(DbCommandType commandType, DbCommand command)
        {
            // if parameters are already set, then return
            // ⚠ In case you set the parameters are already set, we cannot "prepare" once more as this would also cause us loosing the cache
            if (command.Parameters.Count != 0)
                return;

            switch (commandType)
            {
                case DbCommandType.SelectChanges:
                    this.SetSelecteChangesParameters(command);
                    break;
                case DbCommandType.SelectRow:
                    this.SetSelectRowParameters(command);
                    break;
                case DbCommandType.DeleteMetadata:
                    this.SetDeleteMetadataParameters(command);
                    break;
                case DbCommandType.DeleteRow:
                    this.SetDeleteRowParameters(command);
                    break;
                case DbCommandType.InsertMetadata:
                    this.SetInsertMetadataParameters(command);
                    break;
                case DbCommandType.InsertRow:
                    this.SetInsertRowParameters(command);
                    break;
                case DbCommandType.UpdateMetadata:
                    this.SetUpdateMetadataParameters(command);
                    break;
                case DbCommandType.UpdateRow:
                    this.SetUpdateRowParameters(command);
                    break;
                case DbCommandType.Reset:
                    this.SetResetParameters(command);
                    break;
                default:
                    break;
            }

                // ⚠ only prepare a command AFTER you set the parameters (names)
            // in case we set parameters, we prepare the command so it can be reused more efficiently
            command.Prepare();
        }

The best way to find out whether your caching actually works is to profile it with something like JetBrains dotTrace and see hof often "Prepare" was called on DbCommand!

[Mimetis]

Ok, maybe adding a static pool is not worth it and can potentially lead to thread collisions

So far, I've get the command cache dictionnary inside the DbSyncAdapter class:

https://github.com/Mimetis/Dotmim.Sync/blob/aab9148d52d530d64775efe2a0996f54168b8f55/Projects/Dotmim.Sync.Core/DbSyncAdapter.cs#L29

So far, it's created for one table per database per SyncAgent No risk to have any thread collision here.

Will update the perf results once the tests are all passed

[gentledepp]

Nice!

1. Note that I originally introduced a var key = new CommandCacheKey(commandType, additionals); class. This avoids unnecessary string allocations and therefore reduces memory pressure. (Most interesting for Xamarin - especially on Android where GC performance is really bad)

2. Also note that in line 172 of your DbSyncAdapter you always override the Transaction of the command:

           if (Transaction != null)
               selectCommand.Transaction = Transaction;

   This removes any performance benefits of ".Prepare()"

3. And: I could not find the line, where you actually call .Prepare() on the DbCommand! Am I missing something? 😶

[Mimetis]

There is no additional benefits to add Prepare() to SQL Server (at least from what i've read so far)

Anyway, I'v added Prepare() call after any call to OpenConnection and transaction set.
Just because Prepare() needs to be set within an Opened connection.

Here are the new results (with cache in DbSyncAdapter and no Prepare() call)

Provider	Without Command caching	With Command caching	Diff	% Perf increase
SqlServerSyncProvider	25m 11s	22m25s	166 s	11 %
MySqlSyncProvider	29m07s	26m32s	155s	9 %

Here are the new results (with cache in DbSyncAdapter and Prepare() call)

Provider	Without Command caching	With command caching / Prepare	Diff	% Perf increase
SqlServerSyncProvider	25m 11s	23m45s	86 s	5.7 %
MySqlSyncProvider	29m07s	24m09s	298s	17 %

The Prepare command gave some better performances, but not sure it's worth it

[gentledepp]

Well, I still don't know whether your implementation is correct then. You can run the tests with JetBrains dotMemory as well and have a look at the "memory traffic" to get an idea if you improved the memory consumption.

But even if: Remember that the difference may be much more signifficant on other platforms - especially mobile ones with constrained memory consumption.

[Mimetis]

Indeed. I don't have any Jetbrains licences, and I don't have (and don't want) Xamarin installed. Let's say for now, it's ok, we will see later if any issues are raised about this new caching system

[Mimetis]

This command caching issue allowed me to look a little bit deeper in the DbSyncAdapter object.

To simplify things, and to be more consistent, I've removed the dependencies to the Connection and the Transaction. Both are now passed as arguments within the methods.

It allows us to use the same DbSyncAdapter (related to a table) on different connections.

For the DbCommand instances used, I've changed the way they are retrieved from the underlying SyncProvider.

I think I was wrong on the last version, since the Prepare() method was called (sometimes) after the parameters values have been set.

Now, it's straightforward and only written in one single place:

internal async Task<DbCommand> PrepareCommandAsync(DbCommandType commandType, DbConnection connection, DbTransaction transaction, SyncFilter filter = null)
{
    // Create the key
    var commandKey = $"{connection.DataSource}-{connection.Database}-{this.TableDescription.GetFullName()}-{commandType}";

    // Get a lazy command instance otherwise get the command from the underlying provider
    var lazyCommand = commands.GetOrAdd(commandKey, k => new Lazy<DbCommand>(() => GetCommand(commandType, filter)));

    // Get the concrete instance
    var command = lazyCommand.Value;

    if (command == null)
        throw new MissingCommandException(commandType.ToString());

    if (connection == null)
        throw new MissingConnectionException();

    if (connection.State != ConnectionState.Open)
        throw new ConnectionClosedException(connection);

    command.Connection = connection;

    if (transaction != null)
        command.Transaction = transaction;

    // Add Parameters
    await this.AddCommandParametersAsync(commandType, command, connection, transaction, filter).ConfigureAwait(false);

    // Testing The Prepare() performance increase
    command.Prepare();

    return command;
}