timgit / pg-boss

Queueing jobs in Postgres from Node.js like a boss
MIT License
2.15k stars 160 forks source link

High CPU usage after upgrade to v10 #475

Closed danilofuchs closed 2 months ago

danilofuchs commented 2 months ago

After upgrading to pg-boss 10.0.1, we are seeing a much higher CPU utilization on our application:

image

And on our DB:

image

danilofuchs commented 2 months ago

We are seeing immensely increased queries per second when using work with pollingIntervalSeconds = 2 (previously newJobCheckInterval = 2000)

Before upgrade: image

After upgrade: image

hudovisk commented 2 months ago

We found the issue. With v10 the following change in worker.js removed the delay call when the interval - duration <= 500. The worker was not yielding and so consuming a lot of resources. After increasing the interval for one of the queues from 0.5s to 1s the cpu usage went back to normal.

diff --git a/src/worker.js b/src/worker.js
index 386dea41..d245c392 100644
--- a/src/worker.js
+++ b/src/worker.js
@@ -74,7 +74,7 @@ class Worker {

       this.lastJobDuration = duration

-      if (!this.stopping && !this.beenNotified && duration < this.interval) {
+      if (!this.stopping && !this.beenNotified && (this.interval - duration > 500)) {
         this.loopDelayPromise = delay(this.interval - duration)
         await this.loopDelayPromise
         this.loopDelayPromise = null
timgit commented 2 months ago

I will patch this. The intention was to bypass creating delay promises with a very short duration, but in hindsight, this was a bad magic value to use. Perhaps 50 or 100ms is a better threshold to use to make that determination. Do you have a preference?

hudovisk commented 2 months ago

The intention was to bypass creating delay promises with a very short duration

I don't see a problem with that, I think that the worker yielding the cpu for a short period of time would be a better experience than not yielding at all. If we want to ensure a minimum delay time I would prefer something like:

if (!this.stopping && !this.beenNotified) {
        this.loopDelayPromise = delay(Math.max(this.interval - duration, 100))
        await this.loopDelayPromise
        this.loopDelayPromise = null
      }

Perhaps 50 or 100ms is a better threshold to use to make that determination

Agree