Repository navigation
Test the registry lookup, register MRU, document case-insensitive names - #382
Conversation
…sitive names The registry this PR originally proposed landed in #324, so what is left is the part around it. Rebased onto develop after the docs rewrite (#372) and the LRUv0 fix (#373). test_evictionAlgo gains a case for create_cache_using_plugin(). Nothing exercised that entry point, which is the one the MINISIM profiler uses and the one that failed in issue #304: it went straight to dlsym(), which cannot see a built-in constructor in a statically linked build because the archive member holding it is never pulled in. The case also pins the case-insensitive matching, since MINISIM callers pass names in whatever case the user typed. MRU and LRUv0 are compiled into the library but were absent from the registry, so no CLI could reach them. LRUv0 was left out earlier because it segfaulted on the first request; #373 fixed that, so it is registered now. Both names are covered by the new test. The SHARDS profiler compared both the profiler name and the algorithm with strcmp(), so --algo=lru was rejected even though MINISIM and cachesim accept it. It uses strcasecmp() now, which makes "names are case-insensitive" true for every mrcProfiler path, as the docs now say. Docs: the extend guide says names are matched case-insensitively and when an algorithm also needs a branch in cache_init.h, the mrcProfiler option table marks --algo as case-insensitive, scripts/README.md says the same for --algos, and the README lists mru. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VD7L6jWd2YzyhfMJxMquc
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: ba7be7fb77
ℹ️ 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".
MRU_evict advanced q_head and then wrote q_head->queue.prev unconditionally. When the evicted object was the only one in the queue, the new head is NULL, so the second miss in a one-object cache (or any eviction that empties the cache, which variable-sized traces can need) segfaulted. q_tail was also left pointing at the freed object. Handle the empty-queue case the way LRU_evict does: clear q_tail when the head runs out. Registering mru in the previous commit is what makes this path reachable from cachesim and the MRC profiler, so fix it here. The new test drives a one-object MRU cache through four misses; it segfaults without the fix and passes with it, including under ASan and LeakSanitizer. Miss ratios on the sample trace are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VD7L6jWd2YzyhfMJxMquc
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41845879d8
ℹ️ 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".
LRUv0_get inserted every miss itself instead of calling cache_get_base, so it never asked can_insert. With lruv0 now reachable from the CLI, -a was silently ignored: on cloudPhysicsIO at 0.1 with -a size and size=4096, LRU's miss ratio is 0.9258 while LRUv0 stayed at its no-admission 0.8051. The custom get also skipped the prefetcher and never counted requests. LRUv0 already sets find, insert and evict, which are what cache_get_base calls, so LRUv0_get now just calls it, as LRU does. With size admission LRUv0 now matches LRU at 0.9258, its miss ratios without admission are unchanged, and the existing LRUv0 test still matches LRU's expected results. The new test attaches a size admissioner and checks that an object over the threshold is never cached; it fails without the fix and passes with it, including under ASan and LeakSanitizer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VD7L6jWd2YzyhfMJxMquc
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45fc5e1121
ℹ️ 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".
The previous commit routed LRUv0_get through cache_get_base, which only evicts on a miss. LRUv0_find, unlike LRU's, adopts the larger size when a hit carries one, and the old LRUv0_get then evicted until the cache was back under its size. Without that loop a 100-byte cache given (1,60), (2,40), (1,80) held 120 bytes and still hit on object 2. Keep cache_get_base for admission, prefetching and request counting, and run the eviction loop after it again. LRUv0's miss ratios on cloudPhysicsIO now match develop's exactly at 0.01, 0.05, 0.1, 0.3 and 0.6 without admission, and it still honors -a. The new test replays that sequence; it fails on the previous commit and passes now, including under ASan and LeakSanitizer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VD7L6jWd2YzyhfMJxMquc
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ffe9ffa7f
ℹ️ 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".
Registering lruv0 made the CLI hand it options its implementation never sees. LRUv0 is the older GLib-based LRU and keeps its own hash table, so it bypasses cache_find_base, cache_insert_base and cache_evict_base, and with them everything built on those paths: admission (45fc5e1 patched that), TTL expiry and default TTLs, and the prefetchers' find and evict callbacks, so OBL never turns prefetching on and PG never builds its graph. Each review round found another of these, and patching them one at a time does not converge; doing it properly means moving LRUv0 onto the shared paths, which is its own change. So go back to what the original PR did and leave lruv0 out, with a comment that gives the actual reason now that the crash is fixed (#373). Revert 45fc5e1 and 4ffe9ff, which only existed to make the registered lruv0 behave, so LRUv0.c is untouched by this branch. The lookup test now checks that lruv0 stays unregistered. This reverts commits 45fc5e1 and 4ffe9ff. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015VD7L6jWd2YzyhfMJxMquc
|
Decision on Registering
These all have one cause. LRUv0 is the older GLib-based LRU and keeps its own hash table, so it bypasses So 38851c7:
The MRU fix (4184587) stays. MRU uses the shared paths, so it doesn't have this problem. Generated by Claude Code |
1a1a11a
changed the title
What does this PR do?
This rebases #317 (by @haochengxia, who stays the author of the first commit) onto current
develop, which it conflicts with, and fixes what has gone stale since. That PR comes from a fork, so this branch replaces it. Once this merges, #317 can be closed.Kept from the original PR
create_cache_using_plugin(), the by-name entry point the MINISIM profiler uses. It broke in issue 304 becausedlsym()cannot see built-in constructors in a static build. The test also checks that names match case-insensitively.mruadded tocacheAlgoRegistry.c. It was compiled in, but no CLI could reach it.lruv0left out of the registry. The comment explaining why is updated: the crash is fixed (fix(LRUv0): make the GLib-based LRU work again, and test it #373), but LRUv0 keeps its own GLib hash table and bypasses the shared find/insert/evict paths. So the CLI would silently ignore admission, TTLs and prefetcher training for it. The lookup test checks that it stays unregistered.scripts/README.mdnote that--algosnames are case-insensitive.Changed while rebasing
mrc_profiler_params_parse()checked the algorithm withstrcmp(..., "LRU"), so--algo=lru --profiler=SHARDSwas rejected. The profiler name was also only accepted asSHARDSorshards. Both checks usestrcasecmp()now, so the docs are correct for every mrcProfiler path.cache_init.h;--algoas case-insensitive.mruis added to the README's list of supported algorithms.MRU_evictwrote through the new, NULL, head and leftq_tailpointing at the freed object. So the second miss in a one-object cache segfaulted, as could any eviction that empties the cache. It now handles the empty queue the wayLRU_evictdoes.For a while this branch also registered
lruv0, with patches to make it honor admission (45fc5e1, 4ffe9ff). Review kept finding more options LRUv0 ignores, all for the same reason, so 38851c7 reverts both patches and takes it back out of the registry.LRUv0.cis not changed by this PR. See the PR comment for details.Type of change
How was it tested?
New tests:
create_cache_by_name: coverslru,fifoandmruin mixed case, and checks thatlruv0is not registered.cacheAlgo_MRU_evict_only_object: segfaults (exit 139) without its fix, and passes under ASan, UBSan and LeakSanitizer.ctest --test-dir _build --output-on-failurepassesBuild is warning-free (CI uses
-Wall -Wextra -Werror). The local Release build has no warnings in the changed files; I did not build with-Werrorlocally.Results
Checklist
clang-format🤖 Generated with Claude Code
https://claude.ai/code/session_015VD7L6jWd2YzyhfMJxMquc