config(sidecar): bump SIDECAR_CACHE_MAX_ENTRIES 10k -> 100k (#13)#14
Draft
vrogojin wants to merge 1 commit into
Draft
config(sidecar): bump SIDECAR_CACHE_MAX_ENTRIES 10k -> 100k (#13)#14vrogojin wants to merge 1 commit into
vrogojin wants to merge 1 commit into
Conversation
Live measurement on unicity-ipfs1.dyndns.org showed the instant-pin cache saturated at 9,995/10,000 entries while only using 90 MB of its 1 GiB byte budget. The entry-count cap was the binding constraint, producing ~33,405 GET /sidecar/blob 404s per 6 h and a 3.3 % hit rate. Bumps the default in all three places: instant_pin_cache.DEFAULT_MAX_ENTRIES, Dockerfile ENV, and docker-compose.yml. README entry-cap row updated. The existing 1 GiB SIDECAR_CACHE_MAX_BYTES cap still bounds memory/disk footprint -- worst case ~9 KB avg/entry * 100k = ~900 MB, well within budget. LRU eviction of in-kubo rows + 503 back-pressure on pending-only saturation are unchanged. No cache-semantics changes.
There was a problem hiding this comment.
Code Review
This pull request increases the maximum cache entry limit (SIDECAR_CACHE_MAX_ENTRIES) from 10,000 to 100,000 across the Dockerfile, docker-compose, documentation, and Python codebase. Feedback highlights that this ten-fold increase could introduce performance and memory bottlenecks during cache eviction, as the current implementation fetches all candidates into memory and lacks a composite index to optimize the query. It is recommended to batch or limit the eviction query and add a composite index on (state, last_accessed_at).
| DEFAULT_CACHE_DIR = "/data/ipfs/sidecar-cache" | ||
| DEFAULT_MAX_BYTES = 1 * 1024 * 1024 * 1024 # 1 GiB | ||
| DEFAULT_MAX_ENTRIES = 10_000 | ||
| DEFAULT_MAX_ENTRIES = 100_000 |
There was a problem hiding this comment.
Bumping DEFAULT_MAX_ENTRIES to 100_000 introduces potential performance and memory bottlenecks in the cache eviction logic under load:
- Memory Footprint in Eviction: In
_make_room_for(lines 371-376), the querySELECT cid, byte_size FROM instant_pin_cache WHERE state = 'in-kubo' ORDER BY last_accessed_at ASCis executed, andcursor.fetchall()is used to load all candidates into memory. With up to 100,000 entries, this can fetch tens of thousands of rows into memory on the write path, causing latency spikes and high memory usage. - Database Query Performance: The query filters by
stateand orders bylast_accessed_at. Without a composite index on(state, last_accessed_at), SQLite may perform a full table scan or filesort, which becomes significantly slower as the table grows to 100,000 rows.
Recommendations for future improvements:
- Batch Eviction / Limit Query: Limit the
SELECTquery in_make_room_forusing aLIMITclause (e.g., only fetch the oldestNentries to evict, or evict in batches). - Composite Index: Add a composite index on
(state, last_accessed_at)ininit_instant_pin_cache_schemato optimize the eviction query.
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
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
Closes #13. Bumps
SIDECAR_CACHE_MAX_ENTRIESfrom10_000to100_000to relieve the entry-count saturation observed onunicity-ipfs1.dyndns.org(9,995/10,000 entries, 90 MB / 1 GiB byte budget, ~33,405GET /sidecar/blob 404/ 6 h, 3.3 % hit rate).Config tuning only -- cache semantics (LRU eviction of
in-kubo, 503 back-pressure for pending-only saturation, 24 h promotion-timeout WARN) are unchanged.Old / new value
DEFAULT_MAX_ENTRIES(Python)10_000100_000SIDECAR_CACHE_MAX_ENTRIES(Dockerfile ENV)"10000""100000"SIDECAR_CACHE_MAX_ENTRIES(docker-compose default)1000010000010000100000Env-var override path (
docker-composesubstitution /run-ipfs.shpassthrough) is unchanged -- operators can still set a custom value.Files changed
nostr-pinner/instant_pin_cache.py--DEFAULT_MAX_ENTRIES+ docstringDockerfile-- baked-in ENV defaultdocker-compose.yml-- compose defaultREADME.md-- env-var documentation tableMemory / footprint estimate
The cache is SQLite-backed with on-disk blob files (not in-memory). Footprint at 100k entries:
SIDECAR_CACHE_MAX_BYTEScapinstant_pin_cache)The 1 GiB byte cap remains the safety net: if average blob size grows, the byte cap will start triggering eviction before 100k entries are reached, exactly as today.
Eviction policy summary (unchanged)
InstantPinCache._evict_until_fits(ininstant_pin_cache.py):in-kuborows until both caps are satisfied.pendingrows remain and the cap is still exceeded, returnSubmitOutcome.CACHE_FULL-> HTTP 503 back-pressure. Pending blobs are never evicted (writer would silently lose durably-acknowledged bytes).SIDECAR_CACHE_PROMOTION_TIMEOUT(24 h) trigger operator WARN logs but are still not auto-aged-out.Test results
nostr-pinner/tests/test_instant_pin_cache.pyparameterizes caps via_build_cache(max_entries=N, max_bytes=N)-- no test asserts the literal10000, so the bump is test-safe.Sandbox limitation: I could not install
pytest/pytest-asyncio/httpxin this environment (nopip, novenv, network-restricted), so the full suite was not executed locally. Validation done:instant_pin_cache.py-- syntax clean,DEFAULT_MAX_ENTRIES == 100000confirmed.grepaudit confirmed no other tests, fixtures, or docs depend on the old10000literal.Please verify CI runs the test suite on this PR.
Verification (after deploy, per issue #13)
Expected:
max_entries == 100000;miss_ratedrops from ~0.97 toward the byte-budget limit.