tedious icon indicating copy to clipboard operation
tedious copied to clipboard

Upgrading to 15.0.0 causes performance issues

Open grigorii-merck opened this issue 2 years ago • 6 comments

Software versions

  • Tedious: 15.0.0
  • SQL Server: Microsoft SQL Server 2017 (RTM-CU29-GDR) (KB5014553) - 14.0.3445.2 (X64) May 29 2022 12:36:31 Copyright (C) 2017 Microsoft Corporation Standard Edition (64-bit) on Windows Server 2016 Standard 10.0 <X64> (Build 14393: ) (Hypervisor)
  • Node.js: 16.13.0

Additional Libraries Used and Versions

  • Sequelize: 6.21.3

Connection configuration

new Sequelize(config.database, config.username, config.password, {
    host: config.host,
    port: config.port,
    isolationLevel: Transaction.ISOLATION_LEVELS.READ_COMMITTED,
    dialect: "mssql",
    dialectOptions: {
      authentication: {
        type: config.domain ? "ntlm" : "default",
        options: {
          domain: config.domain || undefined,
          userName: config.username,
          password: config.password,
        },
      },
      options: {
        instanceName: config.instanceName,
        appName: "backend",
        enableArithAbort: true,
        timeZone: "UTC",
        requestTimeout: 60000,
        connectTimeout: 15000
      },
    },
    benchmark: true,
    logging: logSql && logOnlyMessageFn,
    pool: {
      min: 10,
      max: 100,
      idle: 15000
    },
  })
}

Problem description Performance degradation after upgrading from 11.8.0. Also high number of time_wait tcp connections but I'm not sure if it's related.

Any other details that can be helpful We did dependencies upgrades and had an issue with performance after upgrading to 14.7.0. Everything became 15-30% slower in tests and requests to db, some of the requests timed out. We noticed that we had an unusually high amount of time_wait tcp connections. We downgraded back to 11.8.0: everything returned to normal, amount of tcp reduced, performance was also okay. Now we gave it another try and upgraded tedious to 15.0.0 - still having the same issue.

grigorii-merck avatar Sep 02 '22 08:09 grigorii-merck

Hi @grigorii-merck , Thanks for raising this up. We will definitely look into this. By the way, have you try to use tedious alone? Does it present the same performance change?

MichaelSun90 avatar Sep 06 '22 16:09 MichaelSun90

have you try to use tedious alone? Does it present the same performance change?

We didn't, as we don't use it alone anywhere in our product. However, regression comes clearly from tedious package upgrade in our case.

badigina-merck avatar Sep 09 '22 07:09 badigina-merck

A lot of things have changed between v15 and v11, especially around packet handling and parsing, and I wouldn't be surprised if a performance regression was accidentally introduced. We do have some benchmarks that we use when developing tedious to make sure performance does not degrade, but sometimes things slip by. 😞

Do you have some more information about the requests you're sending, and the responses you're getting back? Things like response structure (how many columns, how many rows, types of the columns, length of the values contained in these columns) would be extremely helpful for us to reproduce this.

arthurschreiber avatar Oct 05 '22 20:10 arthurschreiber

@arthurschreiber

We get this issue for all requests, including simple login. In login request we only do

userModel.findOne({ where: { username } })

we have index on users table

create index active_users
    on Users (username, status)
go

badigina-merck avatar Oct 06 '22 14:10 badigina-merck

Here's a super simple benchmark:

const { createBenchmark, createConnection } = require('../common');

const { Request } = require('../../src/tedious');

const bench = createBenchmark(main, {
  n: [10, 100, 1000]
});

function main({ n }) {
  createConnection(function(connection) {

    let i = 0;
    bench.start();

    (function cb() {
      const request = new Request('SELECT 1', (err) => {
        if (err) {
          throw err;
        }

        if (i++ === n) {
          bench.end(n);

          connection.close();

          return;
        }

        cb();
      });

      connection.execSql(request);
    })();
  });
}

Running this on Node.js v16.18.0, against the current master branch of tedious:

query/select-1.js n=10: 579.7763570680246 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-1.js n=100: 929.3189295714254 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-1.js n=1000: 1,176.2187528813683 (minor: 4 - 9.087224000133574ms, major: 0 - 0ms, incremental: 0 - 0ms)

Running the same benchmark against the v11.8.0 release:

query/select-1.js n=10: 561.1345107319501 (minor: 0 - 0ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-1.js n=100: 925.7121549893941 (minor: 1 - 1.5373039999976754ms, major: 0 - 0ms, incremental: 0 - 0ms)
query/select-1.js n=1000: 1,273.3212835884297 (minor: 4 - 13.438431000104174ms, major: 0 - 0ms, incremental: 0 - 0ms)

There's a slight variation between versions, but nothing too major that would explain the 15-30% of performance decrease you're describing. 🤔

Can you give me the row definitions of your users table, and maybe a single example row (obviously without any private data)? Just so I can build a benchmark that mirrors that.

Additional notes: The benchmark ran against a local SQL Server 2019 instance. It used a TCP connection, with TLS encryption, and otherwise default connection options.

arthurschreiber avatar Oct 16 '22 16:10 arthurschreiber

Also, would you be so kind to try some of the other versions between v11.8.0 and v14.7.0 to pinpoint which release introduced the performance regression for you? 🙇‍♂️

arthurschreiber avatar Oct 16 '22 16:10 arthurschreiber