Skip to content

Commit cbe577e

Browse files
FrancescAltedclaude
andcommitted
Stop reading a chunk back to find out what is already in it
Filling a chunk block by block rewrites it on every fetch, which is quadratic in bytes moved: half of it building the chunk, half of it reading the previous one back out of the cache and taking it apart again. The write half cannot go. Deferring it to close, which is what the note here proposed, returns zeros: the cache container is what the next read comes out of, so a chunk that is not written is a chunk that is not there. The read-back half can. Proxy now keeps the blocks of the last few partly filled chunks, so the rewrite splices from what it already has. On 64 one-block fetches into a 6.89 MB chunk, 441 MB moved becomes 224 MB and 0.096s becomes 0.054s; memory is bounded by BLOCK_HOT_CHUNKS, which at 8 is what a fetch of 8 whole chunks already peaks at. Evicted chunks fall back to the read-back path, and a whole chunk arriving drops what was held about it -- both covered by tests, since neither shows up in ordinary use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 66f9bfe commit cbe577e

3 files changed

Lines changed: 102 additions & 19 deletions

File tree

‎plans/fsspec-blocks.md‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,18 @@ A.3a as recommended. What the code does differently from the design below:
2828
- Measured end to end afterwards with `--moto`: on 13 MB chunks of 134 blocks, a
2929
point read moves 0.10 MB instead of 13.15 MB — 3.1x faster over an in-region
3030
network, 7.0x over a transatlantic one, 1.0x when the slice wants every block.
31+
- **Filling one chunk a slice at a time is quadratic in bytes moved, and half of
32+
that was removed rather than deferred.** Each fetch that adds blocks rewrites
33+
the chunk; the write cannot be deferred at all, because the cache container is
34+
what the next read comes out of (verified: skipping the write returns zeros).
35+
What could go is the *read-back*: `Proxy` keeps the blocks of the last
36+
`BLOCK_HOT_CHUNKS` partly filled chunks, so a rewrite no longer takes the
37+
cached chunk apart to find out what is in it. Measured on 64 one-block fetches
38+
into a 6.89 MB chunk: 441 MB moved becomes 224 MB, 0.096 s becomes 0.054 s, and
39+
the cache file stays at 1.00x the live bytes (a growing chunk in the middle of
40+
a frame leaves no dead space). The remaining half is marked `ponytail:` and
41+
would need the cache to hold blocks apart from their chunk — a different
42+
container, not a smaller change.
3143

3244
Route B (the io-callback bridge) stays unbuilt, and stays the answer for the
3345
formats route A cannot reach.

‎src/blosc2/proxy.py‎

Lines changed: 47 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,13 @@
5252
# few unwanted bytes for one less request, which at object-store latencies is
5353
# always the right way round.
5454
BLOCK_GAP = 4096
55+
# How many partly filled chunks keep their blocks in memory as well as in the
56+
# cache. Adding a block to a chunk rewrites that chunk, and the blocks already
57+
# in it have to come from somewhere: from here, or read back out of the cache and
58+
# taken apart again. Keeping the last few saves half the copying for the pattern
59+
# that costs the most -- one chunk filled a slice at a time -- and 8 of them is
60+
# the memory a fetch of 8 whole chunks already peaks at.
61+
BLOCK_HOT_CHUNKS = 8
5562

5663
# `jit` kwargs that tune *how* an expression is evaluated, not what container the
5764
# result is stored in. Unlike storage kwargs (`cparams`, `chunks`, `urlpath`, ...),
@@ -354,6 +361,9 @@ def __init__(
354361
for name in ("wants_blocks", "chunk_layout", "block_plan", "read_range")
355362
)
356363
self._blocks_per_chunk = self.src.blocks_per_chunk if serves_blocks else 1
364+
# Blocks of the last few partly filled chunks, so a rewrite need not read
365+
# the chunk back out of the cache to find out what is already in it
366+
self._hot_payloads = {}
357367
nchunks = self._schunk_cache.nchunks
358368
nbits = nchunks * self._blocks_per_chunk
359369
self._fetched = bytearray((nbits + 7) // 8) if fresh else self._load_fetched(nchunks)
@@ -582,8 +592,7 @@ def fetch(
582592
missing = self._missing_chunks(item)
583593
try:
584594
for nchunk, chunk in self._get_chunks(missing, max_concurrency):
585-
self._schunk_cache.update_chunk(nchunk, chunk)
586-
self._mark_fetched(nchunk)
595+
self._store_chunk(nchunk, chunk)
587596
finally:
588597
if missing:
589598
self._save_fetched()
@@ -645,8 +654,7 @@ def fetch_one(task):
645654
try:
646655
for (nchunk, run), data in zip(tasks, self._run(fetch_one, tasks, max_concurrency), strict=True):
647656
if run is None:
648-
self._schunk_cache.update_chunk(nchunk, data)
649-
self._mark_fetched(nchunk)
657+
self._store_chunk(nchunk, data)
650658
continue
651659
payloads = pending.setdefault(nchunk, {})
652660
for nblock, offset, size in run[2]:
@@ -661,22 +669,43 @@ def fetch_one(task):
661669
return self._cache
662670

663671
def _write_blocks(self, nchunk: int, payloads: dict[int, bytes], header: bytes) -> None:
664-
"""Put the blocks just fetched into the cache, keeping those already there."""
672+
"""Put the blocks just fetched into the cache, keeping those already there.
673+
674+
Every fetch that adds blocks to a chunk rewrites that chunk, so filling
675+
one a slice at a time is quadratic in bytes moved. It stays cheap --
676+
compressed bytes, and a rewrite never exceeds what fetching that chunk
677+
whole would have downloaded once -- and the write itself cannot be
678+
deferred, since the cache container is what the read comes out of. What
679+
can be avoided is the other half: `_hot_payloads` keeps the blocks of the
680+
last few partly filled chunks, so they need not be read back out of the
681+
cache and taken apart on every rewrite.
682+
"""
665683
nblocks = self._blocks_per_chunk
666-
held = [n for n in range(nblocks) if self._is_fetched(nchunk, n)]
667-
# ponytail: every fetch that adds blocks to a chunk rewrites it, so N
668-
# separate fetches into the same chunk copy O(N^2) compressed bytes. Cheap
669-
# -- memcpy, never more per rewrite than downloading that chunk once --
670-
# until something pokes one big chunk many times over; then hold the
671-
# payloads per chunk for the session and splice on close instead.
672-
if held:
673-
# The chunk in the cache was spliced by an earlier fetch, so its blocks
674-
# come back out the way they went in, compressed and without a copy of
675-
# anything else
676-
payloads = _chunk_payloads(self._schunk_cache.get_chunk(nchunk), nblocks, held) | payloads
677-
self._schunk_cache.update_chunk(nchunk, _splice_chunk(header, nblocks, payloads))
684+
kept = self._hot_payloads.pop(nchunk, None)
685+
if kept is None:
686+
# Not held any more (or not by this run): the chunk in the cache was
687+
# spliced by an earlier fetch, so its blocks come back out the way
688+
# they went in, compressed and without a copy of anything else
689+
held = [n for n in range(nblocks) if self._is_fetched(nchunk, n)]
690+
kept = _chunk_payloads(self._schunk_cache.get_chunk(nchunk), nblocks, held) if held else {}
691+
kept.update(payloads)
692+
# ponytail: the write half stays quadratic -- a rewrite hands the cache
693+
# the whole chunk, and it cannot be deferred or batched further, since the
694+
# cache is what the next read comes out of. Removing it needs the cache to
695+
# hold blocks apart from their chunk, which is a different container.
696+
self._schunk_cache.update_chunk(nchunk, _splice_chunk(header, nblocks, kept))
678697
for nblock in payloads:
679698
self._mark_fetched(nchunk, nblock)
699+
if len(kept) < nblocks: # a chunk that is now complete will never be rewritten
700+
self._hot_payloads[nchunk] = kept
701+
while len(self._hot_payloads) > BLOCK_HOT_CHUNKS:
702+
self._hot_payloads.pop(next(iter(self._hot_payloads)))
703+
704+
def _store_chunk(self, nchunk: int, chunk: bytes) -> None:
705+
"""Put a whole chunk in the cache, dropping anything held about its blocks."""
706+
self._schunk_cache.update_chunk(nchunk, chunk)
707+
self._mark_fetched(nchunk)
708+
self._hot_payloads.pop(nchunk, None)
680709

681710
async def afetch(
682711
self, item: slice | list[slice] | None = (), max_concurrency: int | None = None
@@ -782,8 +811,7 @@ async def _fetch_one(nchunk):
782811
async with semaphore:
783812
chunk = await self.src.aget_chunk(nchunk)
784813
# Runs to completion between awaits, so concurrent writers can't interleave.
785-
self._schunk_cache.update_chunk(nchunk, chunk)
786-
self._mark_fetched(nchunk)
814+
self._store_chunk(nchunk, chunk)
787815

788816
if to_fetch:
789817
try:

‎tests/test_fsspec.py‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -902,3 +902,46 @@ def test_lazy_blocks_serve_a_single_block_chunk(monkeypatch, any_chunk_wants_blo
902902

903903
assert np.array_equal(p[0:2], data[0:2])
904904
assert len(chunks) == 1
905+
906+
907+
def test_lazy_blocks_reuse_what_they_just_wrote(monkeypatch, any_chunk_wants_blocks):
908+
# Growing a chunk block by block must not read it back out of the cache each
909+
# time; the blocks already in it are still in hand
910+
data, a = _incompressible((200, 200), (100, 200), (10, 20))
911+
p = blosc2.open(_put("hot.b2nd", a), lazy=True)
912+
read_back = []
913+
orig = p.schunk.get_chunk
914+
monkeypatch.setattr(p.schunk, "get_chunk", lambda n: (out := orig(n), read_back.append(n))[0])
915+
916+
for col in range(0, 200, 20):
917+
assert np.array_equal(p[0:5, col : col + 5], data[0:5, col : col + 5])
918+
assert not read_back
919+
920+
921+
def test_lazy_blocks_survive_eviction(monkeypatch, any_chunk_wants_blocks):
922+
# More chunks in flight than the hot cache holds: the evicted ones fall back
923+
# to reading the chunk back, which must reconstruct exactly the same blocks
924+
monkeypatch.setattr(blosc2.proxy, "BLOCK_HOT_CHUNKS", 2)
925+
data, a = _incompressible((400, 200), (100, 200), (10, 20))
926+
p = blosc2.open(_put("evict.b2nd", a), lazy=True)
927+
928+
for chunk_row in range(0, 400, 100): # touch every chunk once, evicting as it goes
929+
assert np.array_equal(p[chunk_row : chunk_row + 5, 0:5], data[chunk_row : chunk_row + 5, 0:5])
930+
# ... then come back to the first, whose blocks are no longer held
931+
assert np.array_equal(p[0:5, 100:105], data[0:5, 100:105])
932+
assert np.array_equal(p[0:5, 0:5], data[0:5, 0:5])
933+
assert np.array_equal(p[...], data)
934+
935+
936+
def test_lazy_blocks_after_a_whole_chunk_arrives(any_chunk_wants_blocks):
937+
# afetch fetches whole chunks, which replaces a partly filled one; what was
938+
# held about its blocks must not be spliced back over it afterwards
939+
import asyncio
940+
941+
data, a = _incompressible((200, 200), (100, 200), (10, 20))
942+
p = blosc2.open(_put("mixed.b2nd", a), lazy=True)
943+
944+
assert np.array_equal(p[0:5, 0:5], data[0:5, 0:5])
945+
asyncio.run(p.afetch((slice(0, 5), slice(None))))
946+
assert np.array_equal(p[0:5], data[0:5])
947+
assert np.array_equal(p[...], data)

0 commit comments

Comments
 (0)