Fix documentation examples and make the Read the Docs build work - #327
Fix documentation examples and make the Read the Docs build work#3271a1a11a wants to merge 1 commit into
Conversation
![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: 48e2983517
ℹ️ 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".
| # size the hash table: --hashpower is log2 of the number of entries, default 24 | ||
| # (16M). Lowering it cuts memory substantially on small traces — replaying the | ||
| # sample trace drops from about 106 MB to 8 MB at --hashpower=16 | ||
| ./bin/cachesim ../data/cloudPhysicsIO.vscsi vscsi lru 1gb --hashpower=16 |
There was a problem hiding this comment.
Users copying this new command get an unrecognized-option error rather than the claimed memory reduction. The complete argp/--help option table in libCacheSim/bin/cachesim/cli_parser.c:55-98 defines no hashpower option, and the CLI never propagates such a value into common_cache_params_t; either expose the setting through cachesim or remove this command and the following comparison advice.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR updates the project’s Markdown documentation to fix broken example commands/paths, correct and modernize API/library documentation, and make Read the Docs builds succeed by adding a Sphinx+MyST pipeline over the existing doc/ Markdown sources.
Changes:
- Replace non-existent sample trace filenames/paths in guides and examples with the shipped
data/cloudPhysicsIO.*traces and consistent./bin/<tool>invocation from the build directory. - Add Sphinx configuration (
doc/conf.py) + RTD config updates to build Markdown via MyST, copy plot/assets into output, and rewrite GitHub-oriented links for a hosted docs site. - Substantially revise API and library docs (e.g.,
doc/API.md,doc/advanced_lib.md,doc/performance.md) to reflect current headers/behavior and remove/repair broken sections.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| FAQ.md | Reformat and expand FAQ; update oracleGeneral struct field and add runnable tool examples/links. |
| doc/requirements.txt | Add doc-only Python dependencies for Sphinx/MyST/RTD theme/mermaid. |
| doc/README.md | Add Help section and link to CONTRIBUTING/FAQ/Issues/Discussions. |
| doc/quickstart_traceUtils.md | Fix heading levels and update example trace paths. |
| doc/quickstart_traceAnalyzer.md | Normalize headings, fix example commands/paths, and repair embedded plot reference/caption. |
| doc/quickstart_mrcProfiler.md | Update commands to ./bin/mrcProfiler and add additional MINISIM notes. |
| doc/quickstart_cachesim.md | Update example commands/paths; refresh algorithm links and add notes on obj-id-is-num and --hashpower. |
| doc/performance.md | Replace placeholder content with concrete throughput/build/trace-format/runtime/memory tuning guidance. |
| doc/memory_usage_profiling.md | Adjust code fence language to avoid misleading shell highlighting for non-shell output. |
| doc/install.md | Normalize heading structure across install guide. |
| doc/index.md | Add MyST toctree-based Sphinx entry point and clarify working-directory assumptions for commands. |
| doc/conf.py | New Sphinx config: MyST Markdown build, copy plot/assets, and rewrite GitHub-style links for RTD. |
| doc/API.md | Rewrite/structure the C API reference with updated types/signatures and guidance. |
| doc/advanced_lib.md | Fix reader and simulator examples and update types/signatures. |
| doc/advanced_lib_extend.md | Fix extension workflow links and improve references to existing readers/files. |
| .readthedocs.yaml | Point RTD at doc/conf.py, install doc/requirements.txt, and enable fail_on_warning. |
| .github/copilot-instructions.md | Update guidance about sample traces and proper benchmarking expectations. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| `--algo` accepts the same names as `cachesim`, so any built-in algorithm works — ARC, S3FIFO, sieve, twoq, and the rest. See the [README](/README.md#supported-algorithms) for the full list. | ||
|
|
||
| `belady` and `beladySize` need a trace that carries future access times, so they only run on an oracle format such as `oracleGeneral` or `lcs`; the profiler says so and stops otherwise. | ||
|
|
||
| `beladySize` is additionally approximate under sampling, beyond the usual sampling error, and warns when you ask for it. It ranks candidates by reuse distance, computed as `next_access_vtime - n_req`, but `next_access_vtime` counts requests in the full trace while `n_req` counts only the requests the sampler kept, so the distance comes out inflated. On `cloudPhysicsIO` at a 100 MB cache, sample rate 0.5 puts it 0.0126 away from the unsampled miss ratio, against 0.0003 for `belady` and 0.0023 for LRU. Use `FIX_RATE,1,<threads>` for an exact run, or `belady`, which compares future times directly and is unaffected. |
48e2983 to
f5a65c7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5a65c7bce
ℹ️ 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".
|
|
||
| `--algo` accepts the same names as `cachesim`, so any built-in algorithm works — ARC, S3FIFO, sieve, twoq, and the rest. See the [README](/README.md#supported-algorithms) for the full list. | ||
|
|
||
| `belady` and `beladySize` need a trace that carries future access times, so they only run on an oracle format such as `oracleGeneral` or `lcs`; the profiler says so and stops otherwise. |
There was a problem hiding this comment.
At this commit, mrcProfiler contains no oracle-format check before MINISIM calls create_cache_using_plugin() and begins replay. With a non-oracle trace such as the documented vscsi sample, requests retain next_access_vtime == -2; debug builds abort only through Belady_get()'s DEBUG_ASSERT, while release builds can continue and report a meaningless MRC. Either add the promised runtime validation in this commit or avoid documenting the profiler as safely rejecting these inputs.
Useful? React with 👍 / 👎.
| ./bin/mrcProfiler ../data/cloudPhysicsIO.vscsi vscsi --algo=FIFO --profiler=MINISIM --profiler-params=FIX_RATE,0.01,10 --size=0.1,0.5,10 | ||
| ``` | ||
|
|
||
| `--algo` accepts the same names as `cachesim`, so any built-in algorithm works — ARC, S3FIFO, sieve, twoq, and the rest. See the [README](/README.md#supported-algorithms) for the full list. |
There was a problem hiding this comment.
MINISIM does not yet accept the same names as cachesim: it constructs <argument>_init and performs a case-sensitive dlsym, whereas cachesim maps names case-insensitively. Consequently, examples explicitly advertised here such as --algo=sieve and --algo=twoq look for nonexistent sieve_init/twoq_init symbols instead of Sieve_init/TwoQ_init and abort. Document the exact symbol-cased names supported by this commit, or land the cachesim-style mapping first.
Useful? React with 👍 / 👎.
f5a65c7 to
9d6a5c6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d6a5c6b3c
ℹ️ 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".
| ## Runtime options | ||
|
|
||
| * `--num-thread=N` — simulations across algorithms and cache sizes are run in parallel, so a sweep costs little more than its slowest single run. | ||
| * `--ignore-obj-size 1` — treats every object as size one. Faster, and the right choice when you want object miss ratio rather than byte miss ratio. |
There was a problem hiding this comment.
When cache sizes represent byte capacity, this advice changes the experiment rather than merely selecting a metric: read_one_req() in libCacheSim/traceReader/reader.c replaces every obj_size with 1, so the configured capacity effectively becomes an object count. cachesim already reports n_miss / n_req as the object miss ratio while retaining real sizes, so users following this recommendation will measure a different cache model and may draw invalid comparisons.
Useful? React with 👍 / 👎.
|
Consolidated response to the review comments on this PR. The csv reader snippet — fixed, and the finding was sharper than it first looked. The snippet used a bare designated initializer, which zeroes every field it does not name; The mrcProfiler guide — moved, not patched. Two reviewers flagged that it describes MINISIM accepting cachesim's algorithm names and warning on sampled
Also fixed here: Generated by Claude Code |
9d6a5c6 to
ae05953
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae059539b2
ℹ️ 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".
|
|
||
| In the sample [cloudPhysicsIO.csv](/data/cloudPhysicsIO.csv), time is in seconds and object size is in bytes. | ||
|
|
||
| `next_access_vtime` is a *logical* time: the number of requests between the current request and the next request to the same object, or `-1` when the object is never accessed again. Algorithms that need future information, such as [Belady](/libCacheSim/cache/eviction/Belady.c) and BeladySize, rely on it, which is why they only work on oracle traces. |
There was a problem hiding this comment.
For users constructing or interpreting oracleGeneral traces from this format description, next_access_vtime is the absolute logical index of the next request, not the number of intervening requests: traceConvOracleGeneral.cpp converts it to stat.n_req - stored_vtime, and Belady compares that value directly with the current request count. Encoding a relative distance as documented changes eviction priorities and can silently produce invalid miss ratios.
Useful? React with 👍 / 👎.
| if target.startswith("../"): | ||
| target = "/" + target[len("../") :] | ||
|
|
||
| return "](%s%s)" % (_REPO_BLOB_URL, target) |
There was a problem hiding this comment.
On the new Read the Docs site, every root-absolute repository directory link is rewritten to a GitHub blob URL, although directories require tree URLs. For example, the newly documented /example and /libCacheSim/include/libCacheSim/ targets become .../blob/develop/example and .../blob/develop/libCacheSim/include/libCacheSim/, so readers cannot open those referenced directories; distinguish directory targets when rewriting them.
Useful? React with 👍 / 👎.
ae05953 to
a3fca4f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3fca4f04e
ℹ️ 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".
| ## Developer Documentation | ||
| - [Debugging Guide](debug.md) | ||
| - [Install & Build](install.md) | ||
| - [Contributing](/CONTRIBUTING.md) |
There was a problem hiding this comment.
Neither CONTRIBUTING.md nor a case variant exists anywhere in the tracked repository (verified with a repository-wide tree search), so this newly added link returns 404 on GitHub; the same nonexistent target is also linked from the new Sphinx index. Add the guide or point both links to an existing contribution resource.
Useful? React with 👍 / 👎.
Part 2 of 5, split out of #324. Independent — mergeable in any order, though see the note at the bottom on ordering against #329.
Documentation only. No source files are touched.
The examples pointed at files that do not exist
doc/referenceddata/trace.vscsi,data/trace.csv,data/trace.txtanddata/trace.oracleGeneralin 45 places. The sample traces aredata/cloudPhysicsIO.*, so every command a new user copied out of a quickstart guide failed on a fresh clone. The README was fixed in #321; this does the same fordoc/.Commands also ran as
./cachesimwith a../datapath that only resolves one directory up from the binary, so they now use./bin/<tool>from the build directory, matching the README.The library examples did not compile
I compiled the reader snippets in
doc/advanced_lib.mdrather than reading them, which found three separate errors:obj_id_fieldset to 6, wherecloudPhysicsIO.csvputs the id in field 5has_headerfalse for a file that has a header.binary_fmt— not a field; it is.binary_fmt_str— with"<3I2H2Q", which fails withunknown format '3'because the format parser has no repeat countsBoth snippets now compile and run, and all three readers agree exactly, which is what makes the corrected column parameters trustworthy:
The docs now also state that
obj_id_is_numdefaults to true. The CLI overrides it to false right after callingset_default_reader_init_params(), socachesimhashes string ids correctly while a library caller taking the defaults gets the opposite — every non-numeric id silently becomes 0, collapsing distinct objects into one and producing a plausible miss ratio rather than an error. Documented rather than changed, since flipping the default would alter behavior for every existing library caller.Read the Docs never built
.readthedocs.yamlpointed Sphinx atdocs/conf.py. There is nodocs/directory, noconf.pyand no.rstanywhere in the repository, so every build failed immediately.Sphinx now runs over the existing Markdown with MyST, so the sources stay readable on GitHub with no duplicated content. Getting to zero warnings from 96, with
fail_on_warningnow set, meant fixing the docs themselves:API.mdhad no headings at all and had drifted badly from the headers,advanced_lib.mddocumented a simulator API that no longer exists, andperformance.mdwas two empty sections and one bullet.The plots
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 — all 18 images 404'd on a docs site.doc/plotanddoc/assetsare now copied into the output and the prefix stripped in the same hook that rewrites the Markdown links.That left one reference unresolved:
twitter_cluster52_10m_popularityDecayLineLog.svghas never existed in the repository, and was inert only because the whole block sat inside an HTML comment. Meanwhilew92_popularityDecayLineLog.svg, generated for exactly that section, sat unreferenced. The section now points at the real file, with a caption rewritten from the plotting script — the old one had been copied from the object-size section and described two plots where the markup had one, and the wrong quantity besides.18 of 18 images now resolve.
Testing
sphinx-build -Wclean on this branch alone: 13 pages, zero warningshref/srcleftovers (0)Ordering
One caveat:
doc/quickstart_mrcProfiler.mddocuments MINISIM behavior that #329 delivers — that it accepts the same algorithm names ascachesim, and thatbeladySizewarns under sampling. If this merges first, that section describes the tool as it will be rather than as it is. Merging #329 first avoids the gap; nothing breaks either way.Generated by Claude Code