perf(worker): reset delays after generating blockTimeout value #2529
Conversation
src/classes/worker.ts
Outdated
| @@ -552,7 +552,7 @@ export class Worker< | |||
| try { | |||
| this.blockUntil = await this.waiting; | |||
|
|
|||
| if (this.blockUntil <= 0 || this.blockUntil - Date.now() < 10) { | |||
| if (this.blockUntil <= 0 || this.blockUntil - Date.now() < 5) { | |||
There was a problem hiding this comment.
in case this operation is too fast, what about using 5 milliseconds
There was a problem hiding this comment.
Yeah. The issue here is that if the moveToActive is called too early it may not return any job. In theory, it should be <= 1 ms to be consistent with the BZPOPMIN minimum timeout value.
There was a problem hiding this comment.
Btw I did not remember we had this hardcoded value here...
There was a problem hiding this comment.
it's coming from b0a13e8
There was a problem hiding this comment.
0.1 was probably a strange value, but why 5 or 10? I think the rationale here is that if the number is under a certain value, like 10ms, then the overhead may be large enough so that when calling moveToActive it will be indeed time to take the next job, but there is a possibility that this is not true in many cases and then you end calling moveToActive without doing anything, and then loop again. So probably a more correct value would be 1ms or 2ms considering our minimum value for BZPOPMIN, what do you think?
There was a problem hiding this comment.
ok, I see you updated already, sorry for the long comment 😅
| @@ -626,6 +626,7 @@ export class Worker< | |||
| ? blockTimeout | |||
| : Math.ceil(blockTimeout); | |||
|
|
|||
| this.updateDelays(); // reset delays to avoid reusing same values in next iteration | |||
There was a problem hiding this comment.
I am concerned about this. Can you express is in a test case where this was a problem before?
There was a problem hiding this comment.
It's a perf change as we can see from these logs that #2466 (comment) zpopmin is called 2 more times after consuming the last delayed marker, as the blockUntil is not reset
There was a problem hiding this comment.
ok, so the next call of BZPOPMIN in that example would have been the default instead of 0.9 or something.
src/classes/worker.ts
Outdated
| @@ -552,7 +552,7 @@ export class Worker< | |||
| try { | |||
| this.blockUntil = await this.waiting; | |||
|
|
|||
| if (this.blockUntil <= 0 || this.blockUntil - Date.now() < 10) { | |||
| if (this.blockUntil <= 0 || this.blockUntil - Date.now() < 5) { | |||
There was a problem hiding this comment.
Yeah. The issue here is that if the moveToActive is called too early it may not return any job. In theory, it should be <= 1 ms to be consistent with the BZPOPMIN minimum timeout value.
src/classes/worker.ts
Outdated
| @@ -552,7 +552,7 @@ export class Worker< | |||
| try { | |||
| this.blockUntil = await this.waiting; | |||
|
|
|||
| if (this.blockUntil <= 0 || this.blockUntil - Date.now() < 10) { | |||
| if (this.blockUntil <= 0 || this.blockUntil - Date.now() < 5) { | |||
There was a problem hiding this comment.
Btw I did not remember we had this hardcoded value here...
roggervalf
changed the title
## [5.7.4](v5.7.3...v5.7.4) (2024-04-21) ### Performance Improvements * **worker:** reset delays after generating blockTimeout value ([#2529](#2529)) ([e92cea4](e92cea4))
ref #2466