denodrivers / redis

🦕 Redis client for Deno 🍕
https://jsr.io/@db/redis
MIT License
454 stars 45 forks source link

Parallel Request Issue #412

Open vfssantos opened 1 year ago

vfssantos commented 1 year ago

I'm experiencing a performance issue with this module, that does not happen using 'npm:redis' module for Deno.

Basically, using a singleton instance

import { connect } from "https://deno.land/x/redis/mod.ts";
const redisClient = await connect({
 hostname: redisHostName,
 port: Number(redisPort),
 password: redisPassword,
});

If I perform many parallel request using this same connection, such as:

const startTime = new Date().getTime()
const promiseResponse = Array(100).fill(0).map(i=>{
  const res = await redisClient.get(cacheKey);
  console.log('time', new Date.getTime() - startTime);
  return res;
});
const res = await Promise.all(promiseResponse);

It looks like the requests are being performed sequentially, instead of in parallel. Here are the "time" logs for a test I've performed:

time 146
time 292
time 153
time 296
time 175
time 330
time 482
time 651
time 797
time 937
time 1082
time 1241
time 1393
time 1544
time 1705
time 1855
time 2006
time 2149
time 2301
time 2453
time 2605
time 2749
time 2900
time 3053
time 3212
time 3359
time 3512
time 3676
time 3836
time 3987
time 4140
time 4292
time 4450
time 4611
time 4762
time 4907
time 5050
time 5195
time 5345
...

The fact that it it consistently increasing indicates that there's something not allowing the requests to be performed in parallel with this lib. However, using the exact same code, but importing the 'redis' module from NPM, as

import { createClient } from "npm:redis";
const redisClient = await createClient({
  url:
    `redis://:${redisPassword}@${redisHostName}:${redisPort}`,
}).connect();
const startTime = new Date().getTime()
const promiseResponse = Array(100).fill(0).map(i=>{
  const res = await redisClient.get(cacheKey);
  console.log('time', new Date.getTime() - startTime);
  return res;
});
const res = await Promise.all(promiseResponse);

Here's the result:

time 536
time 536
time 536
time 536
time 536
time 536
time 536
time 535
time 535
time 535
time 535
time 535
time 532
time 532
time 532
time 532
time 523
time 523
time 523
time 523
time 523
time 522
time 523
time 523
time 523
time 523
time 523
time 523
time 523
time 523
time 523
time 523
time 523
time 661
time 661
time 661
time 661
time 661
time 661
time 662
time 661
time 661
time 661
time 661
time 661
time 661
time 661
time 661
time 661
time 661
time 661
time 660
time 675
time 675
time 673
time 673
time 673
time 672
time 672
time 672
time 672
time 672
time 672
time 673
time 673
time 673
time 673
time 673
time 673
time 673
time 672
time 672
time 672
time 672
time 672
time 671
time 671
time 671
time 671
time 671
time 683
time 683
time 195
time 203
time 203
time 202
time 203
time 203
time 203
time 203
time 202
time 210
time 211
time 211
time 210
time 210
time 210
...

Which clearly indicates that in fact the request ar being performed in parallel.

Is this the expected behavior of this module, or am I missing something here; or this behavior is indeed something which was not expected in this module?

uki00a commented 11 months ago

@vfssantos Thanks for the feedback!

node-redis implements Auto-Pipelining and Connection pooling. In contrast, deno-redis does not yet implement these features. Therefore, deno-redis basically processes commands sequentially.

We would like to support these features in deno-redis as well, although it will take some time. :pray:

esroyo commented 1 month ago

Just want to point-out that the node-redis client has by default a single connection, unless you create the client passing specific isolationPoolOptions (see https://github.com/redis/node-redis/issues/2720).

Therefore I guess the distinguishing feature is the auto-pipelining.

Implementing a connection pool outside of the client itself is trivial, using the same generic-pool lib that node-redis uses:

import { connect, type Redis } from "https://deno.land/x/redis/mod.ts";
import { createPool } from "npm:generic-pool";

const redisPool = createPool<Redis>({
    create: async () => connect({ hostname: "127.0.0.1", port: "6379" }),
    destroy: async (client) => client.close(),
}, { max: 20, min: 1 });

const startTime = new Date().getTime();
const promiseResponse = Array(100).fill(0).map(async () => {
  const redisClient = await redisPool.acquire();
  const res = await redisClient.get("foo");
  await redisPool.release(redisClient);
  console.log("time", new Date().getTime() - startTime);
  return res;
});
const res = await Promise.all(promiseResponse);

// Drain the pool during shutdown
await redisPool.drain();
await redisPool.clear();

It does indeed perform better, at the cost of more connections. Personally I'm fine implementing the pool myself, I like to keep the client simple :shrug:


EDIT: BTW, on node-redis auto-pipelining takes effect for commands executed exactly during the same event loop tick. I'm not sure it is that common to send many different commands on the same exact tick. Wouldn't in an HTTP server each request get handled on a different tick? :thinking: May be auto-pipelining gets more important for scripting scenarios, but for that you may use explicit pipelining, right?