DM-56097: Improve mremove/mexists performance - #157
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
+ Coverage 83.48% 83.98% +0.50%
==========================================
Files 38 39 +1
Lines 8167 8319 +152
Branches 981 990 +9
==========================================
+ Hits 6818 6987 +169
+ Misses 1048 1040 -8
+ Partials 301 292 -9 ☔ View full report in Codecov by Harness. |
timj
changed the title
timj
commented
Sep 15, 2026
timj
marked this pull request as ready for review
September 15, 2026 21:50
dhirving
approved these changes
Sep 18, 2026
dhirving
left a comment
dhirving
left a comment
Contributor
There was a problem hiding this comment.
I agree that if process pools are strictly worse than threads, we just shouldn't use them.
Generally speaking I am not a fan of the way the AI generates unit tests -- I don't think we need extensive brittle mocked tests to verify internal implementation details that aren't that critical.
dhirving
reviewed
Sep 18, 2026
Split the memoized lookups so that a caller can supply an upper bound, and add a flag that a pool worker sets to report a single worker. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The worker count for a subprocess is now set by a pool initializer, so it no longer depends on whether the parent had already calculated its own count and behaves the same under the fork and spawn start methods. Also stop dropping the caller's num_workers argument to mexists() when a process pool is in use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Schemes backed by a connection pool keep the modest default; file URIs hold no pool and so raise their bound. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each batch reports every URI independently, so one failure does not prevent the removal of the URIs that follow it in the same batch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Benchmarking local removal showed that extra threads add contention rather than throughput, because unlink on a local filesystem is not latency bound. The per-scheme bound remains available for a scheme that measures otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Under the spawn start method each worker pays a full interpreter startup, and the cost grows with the number of modules the calling process has imported. A pool is now kept alive and handed to later calls, and is discarded if it breaks. Thread pools are still created per call, since they are cheap and holding one open would keep its threads alive for no benefit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Existence checking now uses the same batching as removal, and the chunking helper is renamed to reflect that it serves both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace the cached pool when the executor class or worker count changes. Shut down the old pool so varying batch sizes do not accumulate workers. Add regression coverage for replacement and shutdown. Co-authored-by: Codex <noreply@openai.com>
Evict broken pools when bulk result handlers catch worker failures. Preserve per-resource failure reports while allowing the next caller to create a healthy pool. Cover existence, removal, transfer, and S3 removal. Co-authored-by: Codex <noreply@openai.com>
Register the cache cleanup hook only when os.register_at_fork exists. Test module loading without the hook and only exercise supported process start methods in worker tests. Co-authored-by: Codex <noreply@openai.com>
Three call sites reduced the worker count to the number of chunks a particular batch produced. With a single cached process pool that makes the pool's size a function of the batch size, so a small call replaces the pool built for a large one and the next large call has to build it again. Under the spawn start method each rebuild costs a full interpreter startup per worker. Asking for more workers than there is work for them costs nothing: ProcessPoolExecutor starts workers on demand for every start method except fork, and under fork a worker is cheap. Generated with AI Co-Authored-By: SLAC AI
Splitting a batch into one URI per chunk meant that checking three files
for existence occupied three workers, and under a process executor could
build a pool of subprocesses to do it. Handing the work over costs more
than the work itself at that size.
Chunks now have a per-scheme floor on their size, and a batch that
produces a single chunk is handled directly by the caller with no
executor involved. The floor is a scheme's property rather than a global
one because it expresses the cost of one operation: a missing-file check
on a local filesystem takes around 100 microseconds, so a batch has to
reach about a hundred URIs before spreading it wins, whereas a scheme
whose every operation is a network round trip is worth overlapping for
two. This generalizes the single-chunk shortcut that S3 bulk removal
already had.
Measured on wekafs with missing files, best of five, milliseconds:
threads process
N before after before after
1 0.37 0.08 20.38 0.07
3 0.54 0.22 20.60 0.25
30 3.14 2.12 23.75 2.28
300 9.56 10.03 31.48 36.31
1000 22.79 21.34 48.10 55.65
10000 270.81 213.09 266.96 259.08
Generated with AI
Co-Authored-By: SLAC AI
A site that sets AWS_REQUEST_CHECKSUM_CALCULATION to WHEN_REQUIRED stops boto3 computing a checksum on upload, so the object metadata carries no CRC32 and test_get_info fails on a developer machine while passing in CI. The fixture already removes the site's credentials and endpoint so that tests cannot reach real infrastructure; these two variables belong with them, because they change what the client does rather than where it points. Generated with AI Co-Authored-By: SLAC AI
timj
force-pushed
the
tickets/DM-56097
branch
from
September 18, 2026 19:14
0ffadca to
cc3ce2f
Compare
Process pools are no faster than threads for remote URIs, and they silently discarded the undo actions that mtransfer registers on a caller's transaction: those were recorded in the worker process and never reached the parent, so a rollback would have left the transferred files in place. Removing them also drops the pool cache, the fork hooks and the worker marking that only process pools needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mtransfer submitted one task per file and sized its pool from the global default. It now groups by the target scheme, so each scheme's own worker bound applies, and gives each worker a batch. Batches are not floored at _min_chunk_size. That floor is for operations cheap enough that handing one over costs more than doing it, which is never true of a transfer, and it would leave workers idle when a few large files are transferred. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The batch size was derived from how many URIs a call was given, which pinned the number of batches and let the batch itself grow without bound. A batch that then drew a run of slow URIs stalled a worker for the rest of the operation with no way to rebalance: with forty slow URIs among forty thousand, a batch of a thousand took 10.3s against a balanced 1.2s, and a batch of twenty-five took 6.5s. Throughput is flat across batch sizes above the point where a batch is worth handing over, so a fixed size per scheme costs nothing and removes the sizing arithmetic. A scheme sets the size from the cost of one of its operations, and transfers get their own because they cost far more than a metadata operation and scale with a file size that is not known up front. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
wsgidav needs bcrypt, which publishes free-threaded wheels no newer than cp314t, so this Python builds it from source. That build fails because the pyo3 behind bcrypt's Rust extension refuses a Python newer than 3.13 and cannot fall back to the limited API on a free-threaded build. The WebDAV tests already skip when wsgidav is missing, reporting that neither WsgiDAVApp nor a test endpoint is available. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
moto mocks S3 by patching botocore in this process, and its stubber reloads the module holding a backend's URL table on every request. Concurrent reloads of the same module race and the loser raises ImportError, which a bulk operation reports as a URI it could not reach. Only a free-threaded interpreter issues those requests at the same time, and which request loses varies from run to run, so the skip covers every test that drives requests in parallel rather than the one that happened to fail. It is conditioned on the interpreter having no GIL rather than on a version, so it stops applying if moto becomes thread safe. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
timj
commented
Sep 18, 2026
| # free-threaded interpreter issues those requests at the same time, and which | ||
| # request loses varies from run to run, so every test that drives requests in | ||
| # parallel is affected rather than one in particular. | ||
| _MOTO_IS_THREAD_SAFE = getattr(sys, "_is_gil_enabled", lambda: True)() |
Member
Author
There was a problem hiding this comment.
So the real fix is to run a moto server instead of in-process (or wait until moto is fixed) but for now we skip the tests in free threaded mode.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note that 3.15t is much faster for mtransfer/mremove than 3.15. Threads is faster for mremove/mexists in all cases over process (even when we optimized process).
Checklist
doc/changes