Skip to content

fix(S3FIFO): size sub-cache hash tables proportionally to avoid OOM #316

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
manishpaulish wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix(S3FIFO): size sub-cache hash tables proportionally to avoid OOM #316

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c8dc889
fix(S3FIFO): size sub-cache hash tables proportionally to avoid OOM
manishpaulish Jun 6, 2026
54ebe8a
fix: correct common_cacheparams_t typo to common_cache_params_t
manishpaulish Jun 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions libCacheSim/cache/eviction/S3FIFO.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,17 @@ static void S3FIFO_evict_main(cache_t *cache, const request_t *req);
// **** ****
// ***********************************************************************

/* Compute initial hashpower for a sub-cache of the given byte size.
* Sizes the table to hold roughly size_bytes/8 entries (8-byte ptr per slot),
* clamped to [1, HASH_POWER_DEFAULT]. */
static inline int s3fifo_hashpower_for_size(int64_t size_bytes) {
if (size_bytes <= 0) return 1;
int hp = 1;
int64_t slots = size_bytes / 8;
while ((1LL << hp) < slots && hp < HASH_POWER_DEFAULT) hp++;
return hp;
Comment on lines +91 to +93

@coderabbitai coderabbitai Bot Jun 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

Suggested change
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.

}

cache_t *S3FIFO_init(const common_cache_params_t ccache_params,
const char *cache_specific_params) {
cache_t *cache =
Expand Down Expand Up @@ -127,11 +138,13 @@ cache_t *S3FIFO_init(const common_cache_params_t ccache_params,

common_cache_params_t ccache_params_local = ccache_params;
ccache_params_local.cache_size = small_fifo_size;
ccache_params_local.hashpower = s3fifo_hashpower_for_size(small_fifo_size);
params->small_fifo = FIFO_init(ccache_params_local, NULL);
params->has_evicted = false;

if (ghost_fifo_size > 0) {
ccache_params_local.cache_size = ghost_fifo_size;
ccache_params_local.hashpower = s3fifo_hashpower_for_size(ghost_fifo_size);
params->ghost_fifo = FIFO_init(ccache_params_local, NULL);
snprintf(params->ghost_fifo->cache_name, CACHE_NAME_ARRAY_LEN,
"FIFO-ghost");
Expand All @@ -140,6 +153,7 @@ cache_t *S3FIFO_init(const common_cache_params_t ccache_params,
}

ccache_params_local.cache_size = main_fifo_size;
ccache_params_local.hashpower = s3fifo_hashpower_for_size(main_fifo_size);
params->main_fifo = FIFO_init(ccache_params_local, NULL);

snprintf(cache->cache_name, CACHE_NAME_ARRAY_LEN, "S3FIFO-%.4lf-%d",
Expand Down