docs: fix broken references and polish README - #321
Conversation
The README's examples pointed at files that are not in the repo, so the documented commands failed when copy-pasted. Fix them and tighten the surrounding prose. Correctness fixes: - point trace examples at the traces that ship in data/ (cloudPhysicsIO.*, twitter_cluster52.csv) instead of the non-existent data/trace.vscsi and data/trace.csv - add obj-id-is-num to the csv examples; without it the reader stops with an error on traces whose object id column is numeric, and note why - fix the debug guide link (/doc/usage.md -> /doc/debug.md), the pre-commit hook path (setup-hooks.sh -> setup_hooks.sh), and the Adaptsize link (adaptsize.c is a directory) - fix the Python Release badge link, which pointed at a different org than the badge image, and the unlinked SIEVE URL - make the library example a compilable translation unit and use default_common_cache_params() so ttl/hashpower are initialized - correct the oracleGeneral struct field name (timestamp -> clock_time) - import SyntheticReader in the Python plugin example, which used a module alias it never defined Content: - list the eviction algorithms that were missing (CAR, LIRS, LRU-K, Clock2QPlus, Random, S3-FIFOd, FIFO-Merge/Reinsertion, ...) and give the name each one is invoked by on the command line - note the build flags for LRB/GLCache/3LCache and that Belady needs an oracle trace - document the Node.js binding, which had a badge but no section - link the documentation index and requirements.txt All commands, links and code samples in the README were executed or resolved against the tree.
Both scripts documented in the README failed on any trace, independent of the arguments given: - plot_mrc_size.py only parsed cachesim output lines starting with "result", but a result line starts with the trace path, so the parsed mrc was always empty and the script exited with "fail to compute mrc". Match on the shape of the result line instead, and tolerate the shorter line printed when object sizes are ignored. - plot_mrc_time.py parsed stdout for the periodic reports, but cachesim emits those as INFO logs on stderr, so the series was always empty and plotting died in ts[-1] with an IndexError. Scan both streams. Also add pandas to requirements.txt; plot_appr_mrc.py imports it.
|
CI is red on The job dies in the
The same job fails identically on I've left it alone since it's unrelated to a README pass and I can't verify a Homebrew change from Linux, but it looks like a one-word fix in Generated by Claude Code |
There was a problem hiding this comment.
Pull request overview
Updates project documentation and supporting Python plotting scripts to ensure README examples, links, and MRC plotting workflows work against files and behaviors that actually exist in-tree.
Changes:
- Fixed
plot_mrc_time.pyto parse periodic cachesim reports from stderr (INFO logs) as well as stdout. - Fixed
plot_mrc_size.pyresult-line detection and made byte-miss-ratio parsing resilient when that field is omitted. - Updated README commands/links/examples (trace paths, CSV params, library example TU, Python plugin import, Node.js section) and added
pandastorequirements.txt.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| scripts/plot_mrc_time.py | Parse periodic INFO reports from stderr in addition to stdout so time-series plotting works. |
| scripts/plot_mrc_size.py | Broaden result-line matching and handle missing byte-miss-ratio output. |
| requirements.txt | Add pandas dependency for plot_appr_mrc.py. |
| README.md | Fix broken links/paths and make examples runnable; expand algorithm list and add Node.js binding section. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| miss_ratio = float(ls[9].strip(",")) | ||
| byte_miss_ratio = float(ls[13].strip(",")) | ||
| # byte miss ratio is not reported when object size is ignored | ||
| byte_miss_ratio = float(ls[13].strip(",")) if len(ls) > 13 else 0.0 | ||
| mrc_dict[algo].append((cache_size, miss_ratio, byte_miss_ratio)) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07ee7b8179
ℹ️ 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".
| * [LFU](/libCacheSim/cache/eviction/LFU.c) `lfu`, [LFU with dynamic aging](/libCacheSim/cache/eviction/LFUDA.c) `lfuda` | ||
| * [ARC](/libCacheSim/cache/eviction/ARC.c) `arc`, [TwoQ](/libCacheSim/cache/eviction/TwoQ.c) `2q`, [CLOCK-PRO](/libCacheSim/cache/eviction/ClockPro.c) `clockpro`, [CAR](/libCacheSim/cache/eviction/CAR.c) `car`, [LIRS](/libCacheSim/cache/eviction/LIRS.c) `lirs`, [Clock2QPlus](/libCacheSim/cache/eviction/Clock2QPlus.c) `clock2qplus` | ||
| * [LRU-K](/libCacheSim/cache/eviction/cpp/LRU_K.cpp) `lru-k`, [LRU-Prob](/libCacheSim/cache/eviction/LRUProb.c) `lru-prob`, [Size](/libCacheSim/cache/eviction/Size.c) `size` | ||
| * [FIFO-Merge](/libCacheSim/cache/eviction/FIFO_Merge.c) `fifo-merge`, [FIFO-Reinsertion](/libCacheSim/cache/eviction/FIFO_Reinsertion.c) `fifo-reinsertion` |
There was a problem hiding this comment.
When users pass the newly documented fifo-reinsertion name, create_cache() registers it as Clock_init in libCacheSim/bin/cachesim/cache_init.h:48, even though FIFO_Reinsertion_init is a separate implementation with different batching and retention defaults. The command therefore succeeds but silently measures Clock, potentially mislabeling experimental results; either map the name to FIFO_Reinsertion_init or omit it from this list.
Useful? React with 👍 / 👎.
| --- | ||
| ## Node.js package | ||
|
|
||
| Node.js bindings are also available, shipping pre-compiled binaries for Linux (x64) and macOS (x64, ARM64). |
There was a problem hiding this comment.
The release workflow only runs on ubuntu-latest, explicitly builds Linux x64, and advertises only Linux x64 in .github/workflows/npm-release.yml:77-80,84-87,113-120; there is no Darwin x64 or ARM64 build job. Consequently macOS installations cannot download either precompiled binary claimed here and instead fall back to node-gyp rebuild, which requires local build dependencies and is not equivalent to the documented precompiled installation experience.
Useful? React with 👍 / 👎.
- plot_mrc_size.py: locate the miss ratio fields by name instead of by
token index. A result line carries two optional fields (byte miss ratio
is dropped when object sizes are ignored, cost saving ratio appears when
the trace has per-request costs), so with --ignore-obj-size on a
cost-bearing trace index 13 held the cost saving ratio and it was
silently recorded as the byte miss ratio:
... miss ratio 0.8327, cost saving ratio 0.0251, throughput 0.93 MQPS
Verified against all six result-line shapes cachesim emits.
- README: drop fifo-reinsertion from the eviction list. cache_init.h maps
that name to Clock_init, so listing it next to FIFO_Reinsertion.c
claimed an implementation the command does not run.
- README: correct the Node.js platform claim. npm-release.yml builds only
Linux x64; other platforms fall back to node-gyp rebuild.
|
All three review findings were valid and are fixed in 51b57c8. Positional
macOS prebuilt binaries — correct, and my mistake: I took the platform list from
Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51b57c8566
ℹ️ 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".
| ## Usage | ||
| ### cachesim (a high-performance cache simulator) | ||
| After building and installing libCacheSim, `cachesim` should be in the `_build/bin/` directory. | ||
| The examples below are run from `_build/`, so the sample traces in [data/](/data/) are at `../data/`. |
There was a problem hiding this comment.
_build working-directory claim
Restrict this statement to the commands that actually use _build, because following it for later cachesim subsections makes the documented debug and plotting commands fail: _build has no ./scripts/debug.sh, and cd scripts also targets a nonexistent _build/scripts directory. Either restore the repository root before those examples or document their paths relative to _build.
Useful? React with 👍 / 👎.
| m = RESULT_REGEX.search(line) | ||
| if m: | ||
| ls = line.split() | ||
| curr_dataname = extract_dataname(ls[0]) |
There was a problem hiding this comment.
Capture the trace path and algorithm from the text preceding cache size rather than using line.split(). When --tracepath contains spaces, cachesim prints that path verbatim, so a valid line such as /tmp/my trace.vscsi LRU cache size ... matches this regex but is recorded with dataname my and algorithm trace.vscsi; the resulting MRC is silently mislabeled.
Useful? React with 👍 / 👎.
- install_dependency.sh: brew has no google-perftools formula (that is the apt name, used correctly in setup_ubuntu), so setup_macOS exited 1 and the macos / clang job failed in Prepare before CMake ran. Homebrew ships tcmalloc as gperftools. Checked the rest of the brew line against the Homebrew API; the other formulae all resolve. - plot_mrc_size.py: split the trace path and algorithm off the text before "cache size" instead of taking tokens 0 and 1. cachesim prints the path verbatim, so a path containing a space was recorded with the wrong dataname and algorithm. - README: the "run from _build/" note applied to the whole cachesim section, but the debug and plotting scripts below it run from the repository root. Scope the note and say where those run.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c962e0469d
ℹ️ 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".
| curr_dataname = extract_dataname(ls[0]) | ||
| m = RESULT_REGEX.search(line) | ||
| if m: | ||
| prefix = m.group("prefix").rsplit(None, 1) |
There was a problem hiding this comment.
When pluginCache is run with an eviction parameter such as cache_name=My Cache, plugin_cache.c preserves that value in cache->cache_name, so cachesim emits ... My Cache cache size .... Splitting the prefix at only the final whitespace records the algorithm as Cache and folds My into the trace name, silently mislabeling the generated MRC; either reject whitespace in custom cache names or parse the trace/algorithm boundary without assuming algorithm names are single tokens.
Useful? React with 👍 / 👎.
doc/ referenced data/trace.vscsi, data/trace.csv, data/trace.txt and data/trace.oracleGeneral in 45 places. The sample traces are data/cloudPhysicsIO.*, so every command a new user copied out of the quickstart guides failed on a fresh clone. The README was fixed in #321; this does the same for doc/. Commands also ran as ./cachesim with a ../data path that only resolves one directory up from the binary, so they now use ./bin/<tool> from the build directory to match the README. The library reader examples in doc/advanced_lib.md did not work either. Compiling them found obj_id_field set to 6 where cloudPhysicsIO.csv puts the id in field 5, has_header false for a file with a header, and .binary_fmt (not a field -- it is .binary_fmt_str) with "<3I2H2Q", which errors with unknown format '3' because the format parser has no repeat counts. Both snippets now compile and agree exactly with the vscsi reader. The docs also now say that obj_id_is_num defaults to true, so a caller with string ids must set it false or every id silently collapses to 0. .readthedocs.yaml pointed Sphinx at docs/conf.py -- there is no docs/ directory, no conf.py and no .rst anywhere, so every build failed immediately. Sphinx now runs over the existing Markdown with MyST, so the sources stay readable on GitHub with no duplication. Getting to zero warnings (from 96, with fail_on_warning set) meant fixing the docs themselves: API.md had no headings and had drifted from the headers, advanced_lib.md documented a simulator API that no longer exists, and performance.md was two empty sections and one bullet. The guides embed their plots with raw <img src="/doc/plot/..."> tags, which resolve on github.com but became site-root paths in the built HTML, so all 18 images 404'd; doc/plot and doc/assets are now copied into the output and the prefix stripped. The popularity-decay section pointed at a plot that has never existed in the repo, which is why it was commented out, while the w92 plot generated for that section sat unreferenced -- it now points at the real file with a caption describing what the plot actually shows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YUq1vM4g82TkaX2jvLmuQc
Summary
A pass over the README. What started as copy-editing turned up that most of the documented commands don't run as written, so this fixes those first and then polishes the prose around them.
Every command, link, and code sample in the README was executed or resolved against the tree (built with Ninja, run against the shipped sample traces).
Broken things this fixes
The examples referenced traces that aren't in the repo.
data/trace.vscsi,data/trace.csv, anddata/twitter_cluster52.vscsidon't exist; the shipped traces aredata/cloudPhysicsIO.*anddata/twitter_cluster52.csv. Every example now points at a real file.The csv examples fail even with the right filename. The reader stops with
detect obj_id is numeric, please specify -t 'obj-id-is-num=1'. Added the option to the csv examples, plus a sentence explaining when it's needed. With it, the csv run reproduces the vscsi run's miss ratio exactly (0.6297), which confirms the documented column mapping is right forcloudPhysicsIO.csv.Both MRC plotting scripts were broken for any input — separate from the README, and the reason the plotting section couldn't be verified as-is:
plot_mrc_size.pyonly parsed cachesim lines starting with"result", but a result line starts with the trace path (printf("%s %s cache size ...", reader->trace_path, ...)). The parsed mrc was always empty, so the script exited withfail to compute mrc.plot_mrc_time.pyparsed stdout for the periodic reports, but cachesim emits those asINFOlogs on stderr. The series was always empty and plotting died withIndexErrorints[-1].Both now produce plots.
plot_appr_mrc.pyimports pandas, which wasn't inrequirements.txt— added.Smaller reference fixes: the debug guide link (
/doc/usage.md→/doc/debug.md), the pre-commit hook path (setup-hooks.sh→setup_hooks.sh), the Adaptsize link (adaptsize.cis a directory), the Python Release badge whose link pointed at a different org than its image, the unlinkedSIEVE(https://sievecache.com), and the oracleGeneral struct field name (timestamp→clock_time, peroracleGeneralBin.h).The C library example wasn't a compilable translation unit (no
main), and{.cache_size=...}leftdefault_ttlandhashpowerzeroed. It now usesdefault_common_cache_params(), matchingtest.c. Verified it compiles with the documentedgccline and prints a miss ratio matchingcachesim.The Python plugin example called
lcs.SyntheticReaderbut never imported anything aslcs. Both Python snippets were run againstpip install libcachesim; the plugin LRU matches the built-in LRU exactly, which is the point of the example.Content added
cachesim— all 29 documented names were run against a trace to confirm they resolve.doc/README.md) andrequirements.txt.traceAnalyzerinvocation in the trace analysis section, which previously had prose but no command.Notes
Structure, tone, and section order are unchanged — this reads as the same README.
Two things I noticed but left alone as out of scope:
doc/quickstart_cachesim.mdanddoc/advanced_lib.mdstill referencedata/trace.vscsi, andcache_init.hhas astrstr(trace_path, "data/trace.")heuristic that lowers hashpower for the sample traces and no longer matches any shipped filename. Happy to follow up on either.Generated by Claude Code