Share one algorithm registry with the MRC profiler, and add --hashpower - #329
Share one algorithm registry with the MRC profiler, and add --hashpower#3291a1a11a wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 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".
| * 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; |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 exposedfind_cache_init_func()/create_cache_by_name()in the public eviction header. - Updated plugin/internal construction to consult the registry first (and return
NULLon lookup/load failures rather than exiting). - Added
--hashpowertocachesimand threaded hashpower into cache creation; updated MINISIM hashpower handling and oracle-policy validation; normalized-1next-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; |
256a188 to
89eb478
Compare
720f1d5 to
aa99429
Compare
89eb478 to
d3dda43
Compare
cddf41e to
74d0d5e
Compare
There was a problem hiding this comment.
💡 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}, |
There was a problem hiding this comment.
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 👍 / 👎.
d3dda43 to
f6189ee
Compare
74d0d5e to
7393ce8
Compare
|
Consolidated response to the review comments on this PR. Cache sizing ignored the sampler's quantization (P1) — fixed, and it was measurable. 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: Rates already of the form 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.
LHD/LRB/3LCache and Generated by Claude Code |
f6189ee to
a65dc8a
Compare
7393ce8 to
7d8f16e
Compare
a65dc8a to
7d460ce
Compare
7d8f16e to
52d2e27
Compare
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=MINISIMaborted on every invocation: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.-rdynamicdoes not help, because the symbol is not in the binary at all. I confirmed it fails identically ondevelop, so this is long-standing rather than new.cachesimmeanwhile carried its own name-to-constructor table. Both now sharecache/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.cconsults the registry first and falls back todlsymonly for genuinely external policies, andcreate_cache_externalreturnsNULLinstead of callingexit().tinyLFUbecomes a plain alias.cachesimused to appendwindow-size=0.01when the caller gave parameters without a window, but WTinyLFU's ownDEFAULT_PARAMSalready 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
--algo=beladyon an ordinary trace ran to completion and printed a curve built from unsetnext_access_vtime, exiting 0, whilecachesim beladyrefuses. 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 suppliesnext_access_vtimewhenever the caller pointsnext_access_vtime_fieldat the right column, and the fouroracleTwr/oracleTwrNSreaders set it too.cachesimto six decimals.cachesimapplies. Peak RSS for an 8-point run: 32.0 MB → 10.2 MB.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
cachesimshrank its hash table when the trace path containeddata/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_initreads zero as "unset" and substitutes the full-size default. Soslruv0 --hashpower=4took 18 MB against 6 MB at 5 — asking for a smaller table allocated a bigger one.Cacheus,S3FIFOd,SLRUv0andLP_SFIFOnow clamp likeHyperbolic,Random,RandomTwoandRandomLRUalready did. The clamp only binds below hash power 8, so behavior at the default is provably unchanged.SFIFOv0divides 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
-1toMAX_REUSE_DISTANCE. A binary trace carrying-1therefore reached Belady, which rejects it outright. Normalized, so the same file read asoracleGeneralor as a plainBIN_TRACEwith"<IQIq"now produces an identical curve.Testing
-Wall -Wextra -Werror;ctest9/9 (testCLIarrives in Add a CLI regression test target #330)cachesimon unsampled runs--hashpowerverified monotonic in memory from 4 through 24, and identical miss ratios at the defaultGenerated by Claude Code