[11.x] Improve performance of Redis queue block_for when a worker has multiple queues to service

[michael-scinocca]

This is a change to attempt to fix/improve #51612

I'm aware this was closed as not a bug, but it is still impacting some people and I believe there is still room for performance improvement here.

The crux of the issue is if you use the redis queue driver with block_for with a worker servicing multiple queues, it will always block for the block_for time before it can get to a job on a secondary (and further) queue. For example - if you configure block_for as 5 and run queue:work with --queue=default,low and are in a situation where you have 0 jobs in default a 100 jobs in low, the current implementation will block on default for 5 seconds each time before it runs a job in low, so we're getting 5 * 100 = 500 seconds of delay while getting through those 100 jobs.

What this change does is track if we had a job to process in our last round of checking the queues and not block in our next check if so. With this, if we got a job from the low queue in our last job round, we will not block on the default queue our next check to remove the delay before getting back to the low queue. If we eventually have no jobs on any queues we will go back to blocking each check. This maintains the efficiency of blocking in most cases without adding a delay if we have jobs to process. This also maintains priority of the default queue - it will still check default first, it just won't block under this condition.

I had to expose the $block out to the Queue\Worker, which I understand is specific to the RedisQueue implementation and not applicable for other connections, however, blocking as a concept is not specific to Redis, it could be applicable for other implementations, so I feel it doesn't muddy the waters.

[taylorotwell]

I think the base Worker having that "block" concept is kinda unfortunate. Feels like a leaky abstraction or something.

[michael-scinocca]

That's fair. The $block parameter existed in the RedisQueue->retrieveNextJob() function, and I have just bubbled it up here. Would renaming $block to $shouldWait in the base Worker help? I don't think any other queue implementations have this kind of wait for signal functionality right now, but it seems like a transferrable concept to me (e.g. use Postgres listen/notify, wait for a file change event from the OS, etc), so having the base Worker inform the Queue implementation if it should attempt to wait if it does implement this functionality seems ok in my opinion. Since the Queue worker is the one managing multiple queues, it was the only appropriate place I could find to put this logic.

Also - I opened this PR after working with PHP and Laravel for one single day (coming from a .NET background of 20 years), so I'm not incredibly well versed in the framework. I was able to make this change locally and it worked really well for me, so I thought I would offer it.

[michael-scinocca]

I've refactored this so the base Worker now only passes in the queue index and the RedisQueue determines entirely if it should block or not. There is new state added in the RedisQueue to track if a secondary queue had a job. When the queue index is 0, the logic will block if this state is clear. The state is then always cleared post block check if the index is 0. It will be set if a job is found on a queue that is not index 0, which will persist until the next time the index is 0 again, upon which it will be checked and then reset. The end result is the queue will block on index 0 if there was no job found on another index in between checks of queue 0.

[it-can]

@michael-scinocca this change seems to give an error with Laravel Horizon see #53063

[michael-scinocca]

@it-can thank you, sorry about that, I've requested a revert.

[it-can]

@it-can thank you, sorry about that, I've requested a revert.

I think this pr should fix it laravel/horizon#1504