fix(S3FIFO): size sub-cache hash tables proportionally to avoid OOM#316
fix(S3FIFO): size sub-cache hash tables proportionally to avoid OOM#316manishpaulish wants to merge 2 commits into
Conversation
|
Too many files changed? Review this PR in Change Stack to see how the pieces fit before you dive in. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughS3FIFO_init now computes a size-based hashpower via a new helper and assigns it to the small FIFO, optional ghost FIFO, and main FIFO before calling FIFO_init for each sub-cache. ChangesS3FIFO Hashpower Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)libCacheSim/cache/eviction/S3FIFO.clibCacheSim/cache/eviction/S3FIFO.c:32:10: fatal error: 'dataStructure/hashtable/hashtable.h' file not found ... [truncated 785 characters] ... x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/lib/clang/18/include" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a helper function s3fifo_hashpower_for_size to dynamically compute the initial hashpower for S3FIFO sub-caches (small, ghost, and main FIFOs) based on their byte sizes. However, a typo was introduced in the S3FIFO_init function signature where common_cache_params_t was misspelled as common_cacheparams_t, which will cause a compilation error.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| return hp; | ||
| } | ||
|
|
||
| cache_t *S3FIFO_init(const common_cacheparams_t ccache_params, |
There was a problem hiding this comment.
There is a typo in the parameter type: common_cacheparams_t should be common_cache_params_t. This will cause a compilation error.
cache_t *S3FIFO_init(const common_cache_params_t ccache_params,![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libCacheSim/cache/eviction/S3FIFO.c`:
- Around line 91-93: The slot count uses truncating division (slots = size_bytes
/ 8) causing underestimates; change to ceil division so sizes just over a
boundary increment slots (compute slots using (size_bytes + 7) / 8 or
equivalent) before the loop that increments hp (the code that checks (1LL << hp)
< slots and compares to HASH_POWER_DEFAULT) so the while loop uses the correctly
rounded-up slot count.
- Line 96: The function signature for S3FIFO_init uses the incorrect type name
common_cacheparams_t; update the parameter type to the correct project type
common_cache_params_t in the S3FIFO_init declaration (and any corresponding
prototype/uses) so the compiler recognizes the type; ensure references to
ccache_params remain unchanged except for the corrected type name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d1bca38-a916-4255-926c-b040bc9aac7b
📒 Files selected for processing (1)
libCacheSim/cache/eviction/S3FIFO.c
| int64_t slots = size_bytes / 8; | ||
| while ((1LL << hp) < slots && hp < HASH_POWER_DEFAULT) hp++; | ||
| return hp; |
There was a problem hiding this comment.
Round up slot count to match the intended ceil behavior.
slots = size_bytes / 8 truncates, so sizes just above a boundary are underestimated (e.g., 17 bytes computes like 16). Use ceil division before the loop.
Suggested fix
- int64_t slots = size_bytes / 8;
- while ((1LL << hp) < slots && hp < HASH_POWER_DEFAULT) hp++;
+ int64_t slots = (size_bytes + 7) / 8;
+ while (hp < HASH_POWER_DEFAULT && (1LL << hp) < slots) hp++;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int64_t slots = size_bytes / 8; | |
| while ((1LL << hp) < slots && hp < HASH_POWER_DEFAULT) hp++; | |
| return hp; | |
| int64_t slots = (size_bytes + 7) / 8; | |
| while (hp < HASH_POWER_DEFAULT && (1LL << hp) < slots) hp++; | |
| return hp; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libCacheSim/cache/eviction/S3FIFO.c` around lines 91 - 93, The slot count
uses truncating division (slots = size_bytes / 8) causing underestimates; change
to ceil division so sizes just over a boundary increment slots (compute slots
using (size_bytes + 7) / 8 or equivalent) before the loop that increments hp
(the code that checks (1LL << hp) < slots and compares to HASH_POWER_DEFAULT) so
the while loop uses the correctly rounded-up slot count.
1 participant
Closes / addresses #137.
Problem
As profiled in #137 by @JasonGuo98, S3FIFO allocates four full-sized hash tables at init time (one per
cache_struct_initcall), each usingHASH_POWER_DEFAULT = 23→ 8M buckets × 8 bytes = 128 MB. For a 1 GB cache this means ~512 MB allocated before a single object is stored.Fix (Option 3 from the issue discussion)
Add a small helper
s3fifo_hashpower_for_size()that computes the minimum hashpower needed for a sub-cache of the given byte size (ceil(log2(size / 8))), clamped to[1, HASH_POWER_DEFAULT].Set
ccache_params_local.hashpowerbefore each of the threeFIFO_initcalls inS3FIFO_init, so each sub-cache starts with a table proportional to its actual configured size fraction rather than the full cache size.Impact (1 GB cache, default 10/90 split)
The small queue reduction alone saves ~112 MB on a 1 GB cache (8×). Larger caches see proportionally larger savings.
No behaviour change —
hashpoweronly affects initial allocation, the table grows on demand as before.Summary by CodeRabbit