[11.x] Optimize PendingBatch@ensureJobIsBatchable - #54485
Conversation
|
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
| } | ||
|
|
||
| if (! in_array(Batchable::class, class_uses_recursive($job))) { | ||
| if (! (static::$batchableClasses[$job::class] ?? false) && ! in_array(Batchable::class, class_uses_recursive($job))) { |
There was a problem hiding this comment.
While it might not possible with class_uses_recursive, this could be taken one step further, by implementing something similar to class_uses_recursive: Caching all classes in the recursive chain 🤔
There was a problem hiding this comment.
Something along the lines of this: 454eb0d...0000530
| { | ||
| foreach (Arr::wrap($job) as $job) { | ||
| if ($job instanceof PendingBatch) { | ||
| $this->ensureJobIsBatchable($job->jobs->all()); |
There was a problem hiding this comment.
@josepostiga Do you think it make sense to remove this check here? If it's a PendingBatch, that all of the jobs added to it should've had this method called for each job.
There is the case where a user creates a PendingBatch and then modifies the $jobs Collection manually ($pendingBatch->jobs->push(new SomeJobThatIsNotDispatchable)), but if we want to guard against this, we would have to ensure this check happens before we write to the repository, probably in PendingBatch@store().
There was a problem hiding this comment.
Ugh, that actually makes sense, so I agree we could drop the check here and assume jobs have been sanitized before.
That case you refer, though, worries me a little. It leaves a "known" inconsistency in the framework. Do you know how hard would it be to re-architect it so that we can also cover for that? I can look into it later, though.
0633f2a to
8851991
Compare
4 participants
Building on top of #54442
We can memoize the classes which use Batchable. I imagine there are more than a few cases where a developer is chaining/batching hundreds of the same job (just for different users or something), so we don't need to recursively check the traits of the same class repeatedly.
What about a long-running process like Octane? Don't you need to clear the cache on application tear down? No, because if the classes change (and now do use
Batchablewhere they previously hadn't) this would require a server restart, which would be clearing them anyways. As an added bonus, we've memoized the failures as well for this use case.