Do not reuse global worker pool for files #84
Conversation
| throw new \ValueError("Worker limit must be a positive integer"); | ||
| } | ||
|
|
||
| $this->pool = $pool ?? new ContextWorkerPool($workerLimit); |
There was a problem hiding this comment.
Shouldn't we deprecate the $workerLimit parameter? We could remove the worker limit from the implementation then, because we have a dedicated pool usually. Actually... shouldn't using a subset of an existing pool be its own pool implementation if really needed?
There was a problem hiding this comment.
Should we emit a deprecation notice if the parameter is passed?
There was a problem hiding this comment.
I think so, yes. I'm looking at adding the pool implementation you suggested above to parallel as well.
trowski
force-pushed
the
no-global-pool-reuse
branch
from
de1f855
to
4066656
Compare
| if (\defined("AMP_WORKER")) { // Prevent spawning infinite workers. | ||
| return new BlockingFilesystemDriver; | ||
| } | ||
|
|
||
| return new ParallelFilesystemDriver; |
There was a problem hiding this comment.
How do we prevent infinite ParallelFilesystemDriver instances now?
There was a problem hiding this comment.
We don't, so tasks can use async file access. @azjezz mentioned this was an issue.
There was a problem hiding this comment.
But inside the worker we'll spawn another worker now and so on and never actually handle filesystem calls, no?
There was a problem hiding this comment.
No no, FileTask which handles file system calls will use the blocking driver for the call. If the worker executes a user task, then another pool of workers might be spawned. I don't see infinite workers being likely (unless someone executes a Task within a Task, but that would be strange), but each worker having its own set of workers would be a possibility. Since by default we limit the number of workers to 8, the resource usage should still be within reason even on systems without eio or uv.
trowski
force-pushed
the
no-global-pool-reuse
branch
from
cd68fe8
to
dbb8ac1
Compare
This modifies the parallel driver to create its own internal pool by default, rather than reuse the global worker pool. A specific pool can still be passed to the constructor (though I assume this use is rather rare).
The
AMP_WORKERconstant is no longer used to prevent the blocking driver from being used. This way, aTaskmay still use async file access and get the behavior expected./cc @azjezz