Skip to content

Share one algorithm registry with the MRC profiler, and add --hashpower - #329

Open
1a1a11a wants to merge 1 commit into
claude/polish-3-algo-fixesfrom
claude/polish-4-registry-hashpower
Open

Share one algorithm registry with the MRC profiler, and add --hashpower#329
1a1a11a wants to merge 1 commit into
claude/polish-3-algo-fixesfrom
claude/polish-4-registry-hashpower

Conversation

@1a1a11a

@1a1a11a 1a1a11a commented Aug 13, 2026

Copy link
Copy Markdown
Owner

Part 4 of 5, split out of #324. Stacked on #328 — the diff shown here is against that branch; merge #328 first.

MINISIM could not run any built-in algorithm

mrcProfiler --profiler=MINISIM aborted on every invocation:

[ERROR] cannot load internal cache FIFO: undefined symbol: FIFO_init

It resolved algorithms with dlsym() against its own executable, which cannot work for a statically linked build — the constructors live in an archive member nothing references, so the linker never pulls them in. -rdynamic does not help, because the symbol is not in the binary at all. I confirmed it fails identically on develop, so this is long-standing rather than new.

cachesim meanwhile carried its own name-to-constructor table. Both now share cache/cacheAlgoRegistry.c, and referencing that table is what pulls the archive members in, so the lookup is a plain function call with no dynamic loading. plugin.c consults the registry first and falls back to dlsym only for genuinely external policies, and create_cache_external returns NULL instead of calling exit().

tinyLFU becomes a plain alias. cachesim used to append window-size=0.01 when the caller gave parameters without a window, but WTinyLFU's own DEFAULT_PARAMS already sets exactly that before applying caller parameters, so the append could only ever re-set a value that was already there — verified identical across all four parameter shapes. Removing the special case is what makes the name reachable from MINISIM.

MINISIM correctness

  • Oracle-only policies were unchecked. --algo=belady on an ordinary trace ran to completion and printed a curve built from unset next_access_vtime, exiting 0, while cachesim belady refuses. A plausible answer is worse than a crash, since nothing signals it is wrong. The profiler now checks, and asks the reader rather than matching on the trace-type enum — the generic binary reader supplies next_access_vtime whenever the caller points next_access_vtime_field at the right column, and the four oracleTwr/oracleTwrNS readers set it too.
  • Unsampled runs measured the wrong cache. Above rate 0.5 MINISIM disables the sampler but still scaled sizes by the rate, so a 100 MB point at 0.75 measured a 75 MB cache (0.8257 where 100 MB gives 0.8225). Unsampled runs are now exact rather than approximate, agreeing with cachesim to six decimals.
  • BeladySize got a 1M-slot table per profile point, because MINISIM builds caches straight from the registry at a fixed hash power of 20 and so skipped the reduction cachesim applies. Peak RSS for an 8-point run: 32.0 MB → 10.2 MB.
  • BeladySize is approximate under sampling and now says so. It scores candidates with next_access_vtime - n_req; the first counts requests in the full trace, the second only those the sampler kept. Measured against the unsampled miss ratio at 100 MB: at rate 0.5, BeladySize is off by 0.0126 where Belady is off by 0.0003 and LRU by 0.0023. Belady is unaffected — it compares future times directly and never takes a difference. Warned and documented rather than corrected, since remapping future times into sampled virtual time is a change to the sampler with its own trade-offs.

--hashpower

cachesim shrank its hash table when the trace path contained data/trace. — a file that has not existed for a long time, so the saving never happened. Repointing the string is not equivalent: sampling-based algorithms draw candidates from the hash table, so RandomLRU and hyperbolic move when it resizes. Sizing a table by sniffing a filename is the wrong mechanism for something that changes results.

Replaced with an explicit --hashpower, default 24 as before. Replaying the sample trace goes from 106 MB to 15 MB at 20 and 8 MB at 16.

Exposing it surfaced a latent bug found in review of #324: several composite policies size their sub-caches by subtracting from the parent's hash power with no lower bound, and cache_struct_init reads zero as "unset" and substitutes the full-size default. So slruv0 --hashpower=4 took 18 MB against 6 MB at 5 — asking for a smaller table allocated a bigger one. Cacheus, S3FIFOd, SLRUv0 and LP_SFIFO now clamp like Hyperbolic, Random, RandomTwo and RandomLRU already did. The clamp only binds below hash power 8, so behavior at the default is provably unchanged.

SFIFOv0 divides by that same expression rather than assigning it, so it divided by zero at hash power 4. I guarded the divisor rather than rewriting it to match its siblings — the division reads as deliberate, and the algorithm is not in the CLI registry, so there was no call to change its sizing on a guess.

Binary reader

The generic binary reader passed the "no next access" sentinel through raw, where the oracle readers normalize -1 to MAX_REUSE_DISTANCE. A binary trace carrying -1 therefore reached Belady, which rejects it outright. Normalized, so the same file read as oracleGeneral or as a plain BIN_TRACE with "<IQIq" now produces an identical curve.

Testing

  • builds clean with -Wall -Wextra -Werror; ctest 9/9 (testCLI arrives in Add a CLI regression test target #330)
  • MINISIM verified working for built-in algorithms, and exact against cachesim on unsampled runs
  • --hashpower verified monotonic in memory from 4 through 24, and identical miss ratios at the default

Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 720f1d5fd4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread libCacheSim/mrcProfiler/mrcProfiler.cpp Outdated
* since the miniature caches are built straight from the registry and would
* otherwise get a 1M-slot table each. Hyperbolic needs no such line because
* Hyperbolic_init already shrinks its own. */
int minisim_hashpower = kMiniSimHashPower;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Use cachesim's hashpower for exact MINISIM runs

When FIX_RATE,1 disables sampling for Random, RandomTwo, or RandomLRU, this still initializes the policies with hashpower 20 while cachesim defaults to 24. Each of those constructors subtracts eight, so MINISIM uses 2^12 buckets while cachesim uses 2^16; because their victim selection samples buckets through the hash mask, the supposedly exact run can produce a different miss curve. Unsampled runs should pass the same default hashpower as cachesim, while retaining the smaller value only for sampled miniature caches.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Correct, and it falsified a claim I had made in this PR's description — fixed.

I had asserted that unsampled MINISIM runs are exact against cachesim, but I had only checked LRU and FIFO, which do not sample the hash table. Checking the policies that do:

algorithm cachesim MINISIM FIX_RATE,1 (before)
randomTwo 0.8221 0.8215
random 0.8197 0.8196
lru 0.8225 0.8225

So the claim held for the algorithms I happened to test and failed for exactly the ones you named.

Unsampled runs now use cachesim's hash power rather than the miniature 20, keeping 20 for genuinely sampled runs. That alone left hyperbolic 0.0001 out, because cachesim reduces it twice — once in cache_init.h and again inside Hyperbolic_init — so the profiler mirrors the cache_init.h reduction for hyperbolic and beladySize as well.

Re-checked across nine algorithms at two sizes each, including every policy that draws candidates through the hash mask:

random      cachesim= 0.8197 0.7658    MINISIM= 0.8197 0.7658    match
randomTwo   cachesim= 0.8221 0.7691    MINISIM= 0.8221 0.7691    match
RandomLRU   cachesim= 0.8227 0.7678    MINISIM= 0.8227 0.7678    match
hyperbolic  cachesim= 0.8186 0.7665    MINISIM= 0.8186 0.7665    match
beladySize  cachesim= 0.6744 0.5537    MINISIM= 0.6744 0.5537    match
lru         cachesim= 0.8225 0.7661    MINISIM= 0.8225 0.7661    match
sieve       cachesim= 0.8101 0.7360    MINISIM= 0.8101 0.7360    match
arc         cachesim= 0.8012 0.7254    MINISIM= 0.8012 0.7254    match
s3fifo      cachesim= 0.8017 0.6854    MINISIM= 0.8017 0.6854    match

"Exact" is now a property I have actually tested rather than one I inferred from two easy cases. testCLI asserts the equality directly, so it cannot drift back quietly.


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR centralizes built-in cache-algorithm name resolution into a shared registry used by both cachesim and the MRC profiler (MINISIM), and adds an explicit --hashpower knob to control hash table sizing. It also tightens MINISIM correctness checks for oracle-only policies and normalizes binary-reader “no next access” sentinels to match oracle readers.

Changes:

  • Added a library-level algorithm registry (cacheAlgoRegistry.c) and exposed find_cache_init_func() / create_cache_by_name() in the public eviction header.
  • Updated plugin/internal construction to consult the registry first (and return NULL on lookup/load failures rather than exiting).
  • Added --hashpower to cachesim and threaded hashpower into cache creation; updated MINISIM hashpower handling and oracle-policy validation; normalized -1 next-access values in the binary reader.

Reviewed changes

Copilot reviewed 6 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
libCacheSim/traceReader/generalReader/binary.c Normalizes next_access_vtime == -1 to MAX_REUSE_DISTANCE for binary traces.
libCacheSim/mrcProfiler/mrcProfiler.cpp Adds reader capability check for Belady policies; adjusts MINISIM sampling/hashpower behavior and warnings.
libCacheSim/include/libCacheSim/evictionAlgo.h Exposes new public APIs for name→constructor lookup and construction.
libCacheSim/cache/plugin.c Uses registry for built-ins; improves error handling for external/internal loading paths.
libCacheSim/cache/CMakeLists.txt Adds the new registry translation unit to the cache library build.
libCacheSim/cache/cacheAlgoRegistry.c New shared registry mapping algorithm names/aliases to constructors.
libCacheSim/bin/MRC/parser_mini.c Threads default hashpower into cache creation for mini/MRC CLI.
libCacheSim/bin/cachesim/internal.h Adds hashpower to CLI argument struct.
libCacheSim/bin/cachesim/cli_parser.c Adds --hashpower option parsing and passes it into cache creation.
libCacheSim/bin/cachesim/cache_init.h Removes local constructor table; delegates to library registry; adds hashpower parameter.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

{"clockpro", ClockPro_init},
{"fifo", FIFO_init},
{"fifo-merge", FIFO_Merge_init},
{"fifo-reinsertion", Clock_init},
exit(EXIT_FAILURE);
WARN("cannot find %s in %s: %s\n", cache_init_func_name, shared_lib_path,
error);
return NULL;
@1a1a11a
1a1a11a force-pushed the claude/polish-3-algo-fixes branch from 256a188 to 89eb478 Compare August 13, 2026 17:50
@1a1a11a
1a1a11a force-pushed the claude/polish-4-registry-hashpower branch from 720f1d5 to aa99429 Compare August 13, 2026 17:50
@1a1a11a
1a1a11a force-pushed the claude/polish-3-algo-fixes branch from 89eb478 to d3dda43 Compare August 13, 2026 18:07
@1a1a11a
1a1a11a force-pushed the claude/polish-4-registry-hashpower branch 2 times, most recently from cddf41e to 74d0d5e Compare August 13, 2026 18:12

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 74d0d5ee37

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

{"fifomerge", FIFO_Merge_init},
{"flashProb", flashProb_init},
{"gdsf", GDSF_init},
{"lhd", LHD_init},

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve a compatible TTL when registering LHD

When built with -DSUPPORT_TTL=ON, the newly registered LHD path still cannot be used by MINISIM: the profiler constructs every cache with default_ttl = 0, while LHD_init aborts whenever the value is below 30 days; cachesim instead supplies 300 days. Consequently, --algo=LHD now resolves successfully but terminates before simulation (and the optional LRB and 3LCache entries have the same guard). Pass the cachesim default or another algorithm-compatible nonexpiring value from MINISIM.

Useful? React with 👍 / 👎.

1a1a11a commented Aug 13, 2026

Copy link
Copy Markdown
Owner Author

Consolidated response to the review comments on this PR.

Cache sizing ignored the sampler's quantization (P1) — fixed, and it was measurable. create_spatial_sampler stores sampling_ratio_inv as an integer, so it only represents rates of the form 1/n: it truncates 1/0.3 to 3 and keeps a third. Sizing used the rate that was asked for rather than the one in force, making every cache 10% too small at 0.3 while the curve was still labelled with the requested size — so the error was invisible in the output.

Demonstrable by asking for two rates that quantize to the same sampler, and therefore sample identical objects, so any difference between them is the bug and nothing else:

before   rate=0.3333333  0.8193 0.7637
         rate=0.3        0.8204 0.7788
after    rate=0.3333333  0.8193 0.7637
         rate=0.3        0.8193 0.7637

Rates already of the form 1/n, including the 0.5 boundary and the 0.01 used throughout the docs, are unaffected.

Unsampled runs used the wrong hash power (P1) — fixed, with the details in the thread. It falsified my "exact" claim, which I had tested only on LRU and FIFO — the two policies that do not sample the hash table. Now verified across nine algorithms including every hash-sampling policy.

plugin.c leaked the dlopen handle on dlsym failure — fixed. Nothing from the library is in use on that path, unlike the success path the existing comment covers, so it is closed rather than leaked.

fifo-reinsertionClock_init — not a regression, and unchanged. develop's own table already maps it that way, so cachesim fifo-reinsertion has always built a Clock cache; the registry is a verbatim move. Repointing it at FIFO_Reinsertion_init would change what every existing invocation simulates, which is the one thing a behaviour-preserving refactor must not do. The genuinely interesting half is that FIFO_Reinsertion_init is reachable under no name at all, on develop or here — either dead as a standalone policy, or wanting its own entry.

LHD/LRB/3LCache and default_ttl — real, not yet fixed. With -DSUPPORT_TTL=ON, MINISIM constructs caches with default_ttl = 0 while LHD_init aborts below 30 days and cachesim passes 300. So registering those names makes them resolvable but still unusable from the profiler in that build. Passing cachesim's default from MINISIM looks right, but it is a build configuration I have not exercised here, so I am flagging rather than guessing.


Generated by Claude Code

@1a1a11a
1a1a11a force-pushed the claude/polish-3-algo-fixes branch from f6189ee to a65dc8a Compare August 14, 2026 20:04
@1a1a11a
1a1a11a force-pushed the claude/polish-4-registry-hashpower branch from 7393ce8 to 7d8f16e Compare August 14, 2026 20:04
@1a1a11a
1a1a11a force-pushed the claude/polish-3-algo-fixes branch from a65dc8a to 7d460ce Compare August 14, 2026 20:16
@1a1a11a
1a1a11a force-pushed the claude/polish-4-registry-hashpower branch from 7d8f16e to 52d2e27 Compare August 14, 2026 20:16
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.

3 participants