fix: keep FusedCodecPipeline compute off the event-loop thread - #4194
Open
d-v-b wants to merge 6 commits into
Open
fix: keep FusedCodecPipeline compute off the event-loop thread#4194d-v-b wants to merge 6 commits into
d-v-b wants to merge 6 commits into
Conversation
* fix: byte-order handling for structured dtypes in the bytes codec The bytes codec neither byte-swapped structured-dtype fields to its configured endian on encode (numpy reports byteorder '|' for void dtypes, so the top-level byteorder comparison never detected a mismatch) nor honored its endian when decoding, silently corrupting any structured data whose field byte order differed from the stored one (e.g. virtual references to external big-endian data). Encode now detects byte-order mismatches by comparing full dtypes via newbyteorder, and decode reinterprets raw bytes in the stored byte order before converting to the data type's declared byte order, so the stored layout (codec state) and the in-memory layout (array data type) are independent. Closes zarr-developers#4141 Assisted-by: ClaudeCode:claude-fable-5 * test: fold structured byte-order cases into existing bytes codec tests Extend test_endian's parametrization with structured dtypes and test_bytes_codec_sync_roundtrip with endian/dtype parametrization plus stored-layout and decoded-dtype assertions, instead of adding parallel test functions for the same properties. Assisted-by: ClaudeCode:claude-fable-5 * refactor: rename stored_dtype to view_dtype in BytesCodec decode The variable is the dtype used to view the raw chunk bytes (byte order from the codec's endian configuration), not a property of the stored data or of the returned buffer, which always carries the array's declared dtype. Assisted-by: ClaudeCode:claude-fable-5 * docs: note that the decode-side byte-order conversion copies the chunk Assisted-by: ClaudeCode:claude-fable-5
…op thread FusedCodecPipeline.read/write ran their synchronous fast path inline on the coroutine servicing the request — i.e. on the global zarr_io event loop thread. Single-chunk batches decoded inline on the loop and multi-chunk batches blocked the loop in pool.map, so every sync-API call from every user thread serialized behind each other's codec compute. The blocked window scales with codec cost, which is why users reported the fused pipeline as "slower for zstd-compressed data" under multi-threaded (dask-style, one chunk per call) access: at 8 reader threads on 4 MiB zstd chunks it was 3.4x slower than BatchedCodecPipeline, and throughput did not scale with threads at all (336 -> 439 ms from 1 to 8 threads, versus 669 -> 121 ms for batched). Offload the synchronous batch to a worker thread with asyncio.to_thread: one hop per batch, not per chunk, preserving the fused pipeline's win over per-chunk async scheduling while keeping the loop free. After the fix the same workload scales 625 -> 109 ms from 1 to 8 threads, beating batched at every thread count; single-threaded performance is unchanged (the hop costs ~75 us per batch). The regression test asserts deterministically (no timing) that codec compute never runs on a thread with a running event loop, covering single- and multi-chunk reads and writes through the sync API. A new benchmark covers the many-threads/one-chunk-per-call access pattern. Assisted-by: ClaudeCode:claude-fable-5
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4194 +/- ##
=======================================
Coverage 93.87% 93.87%
=======================================
Files 91 91
Lines 12594 12594
=======================================
Hits 11822 11822
Misses 772 772
🚀 New features to boost your workflow:
|
d-v-b
marked this pull request as ready for review
July 28, 2026 14:41
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
1 participant
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.
Summary
FusedCodecPipeline.readandFusedCodecPipeline.writerun on the event loop thread, which means CPU-bound code it spins up can choke out concurrent awaitables queued in the event loop. This PR instead makesreadandwritesend work to a thread viaasyncio.to_thread, which offers a performance win for theFusedCodecPipeline.Although the perf we get here is good, the pattern is problematic: the codec pipeline is taking charge of how its resource-intensive routines are being scheduled in a manner that is opaque to a caller. There are a few better ways of solving the fundamental problem here, but they require classes that look different.
For reviewers
Please check to ensure that this change isn't problematic from a performance POV.
Author attestation
TODO
docs/user-guide/*.mdchanges/based on work by claude: see d-v-b#247