Skip to content

perf(imaging): append to the cache outside the global lock, not inside it - #409

Merged
sebasmos merged 0 commit into
mainfrom
perf/cache-append-outside-the-lock
Jul 31, 2026
Merged

perf(imaging): append to the cache outside the global lock, not inside it#409
sebasmos merged 0 commit into
mainfrom
perf/cache-append-outside-the-lock

Conversation

@sebasmos

@sebasmos sebasmos commented Jul 31, 2026

Copy link
Copy Markdown
Member

CORRECTION, added after merge. This PR led with a storage diagnosis and named OneDrive as the
likely cause. That was wrong for @MohShahin's machine: he confirmed its storage is local disk.
His own read, a missing per-request network timeout, is the better one, and it is what #370 fixes
(GeminiBackend now takes a 60s deadline; merged 22:27). The change in this PR is still correct
and worth having on any filesystem, since holding a global lock across a file write serialises
every worker behind it regardless of where the file lives. But the "put the cache on local disk"
recommendation below did not apply to him, and storage should not have been my lead. Leaving the
original text intact so the reasoning is auditable.

For @MohShahin, who is seeing 1.7 calls/min against the 14-24/min the same arms managed when healthy.

The bottleneck

Every imaging runner did this:

with _lock:
    self.store[k] = resp
    self.calls += 1
    with open(self.path, "a") as f:      # <-- file write, holding the global lock
        f.write(...)

All four workers serialise behind that append. On synced or network storage the write can block for seconds, so throughput caps at roughly one call per append no matter what max_workers says. That matches the symptom: a rate an order of magnitude below healthy, varying over time rather than steadily, on a lane where an earlier cache corruption was already traced to OneDrive.

The dict update needs the lock. A single short line appended in "a" mode does not.

What this does not change

  • Durability: the write still completes before the call returns. A crash loses nothing it would not have lost before. No batching, deliberately, given the corruption history on this lane.
  • Cache keys: untouched, so no existing entry is invalidated. Safe to apply mid-run.
  • Numbers: the committed judge arm still replays at 0 new API calls with identical output.

10 runners changed, mechanically and identically. 1145 passed, 7 skipped, ruff clean.

And it is not the whole answer

Putting the cache on genuinely local disk is still the first thing to try, and it needs no code. This PR removes the amplifier; the storage is the cause.

Two other things worth checking before spending more hours, neither of which this PR touches:

  1. Retry backoff. RetryBackend(..., tries=5, backoff=3.0) burns 3+6+12+24s on a throttled call, which alone floors four workers near 5/min. The signature is long stretches where the cache does not grow at all.
  2. No downscaling. Every call uploads a full-resolution grayscale PNG. Do not change this mid-run: the cache key hashes the image bytes, so resizing invalidates all 1423 entries already paid for. It is the right change for the next clean run.

Bigger lever than any of this

solo needs ~4170 calls and is 34% done. The judge cell in #393 needs referee_300/imaging_cascade.jsonl, which is 417 calls. Running that arm first gets the blocked cell in a tenth of the work.

@sebasmos
sebasmos merged this pull request into main Jul 31, 2026
@sebasmos
sebasmos deleted the perf/cache-append-outside-the-lock branch July 31, 2026 19:16
sebasmos added a commit that referenced this pull request Aug 4, 2026
…he-lock

perf(imaging): append to the cache outside the global lock, not inside it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant