Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the MultiQueue (MQ) cache eviction algorithm, a frequency-aware policy designed to enhance cache performance by managing objects across multiple LRU queues based on their logarithmic frequency. This new algorithm expands the simulator's capabilities, offering a configurable multi-LRU design to complement existing eviction strategies. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the Multi-Queue (MQ) eviction algorithm, a frequency-aware policy. The implementation is well-structured and follows the existing patterns in the codebase. However, there are several critical areas that need attention. Most importantly, the PR lacks unit tests for the new algorithm. Given that the author was unable to perform a full compile and test run due to environment issues, adding comprehensive tests in test/test_evictionAlgo.c is crucial to verify the correctness and prevent regressions. My review also identified several high-severity issues in MultiQueue.c, including missing NULL checks for memory allocations and a portability issue with a hardcoded metadata size. These should be addressed before merging.
| cache->get_n_obj = cache_get_n_obj_default; | ||
|
|
||
| if (ccache_params.consider_obj_metadata) { | ||
| cache->obj_md_size = 8 * 2; |
There was a problem hiding this comment.
The metadata size is hardcoded as 8 * 2. This assumes a pointer size of 8 bytes, which is not portable across different architectures (e.g., 32-bit systems). It's better to use sizeof(cache_obj_t *) * 2 to calculate the size of the prev and next pointers dynamically, ensuring portability.
| cache->obj_md_size = 8 * 2; | |
| cache->obj_md_size = sizeof(cache_obj_t *) * 2; |
| cache->obj_md_size = 0; | ||
| } | ||
|
|
||
| cache->eviction_params = malloc(sizeof(MQ_params_t)); |
There was a problem hiding this comment.
The memory allocated by malloc is not checked for NULL. If malloc fails, this will result in a NULL pointer dereference later, causing a crash. Please add a check for the return value and handle the error, for example by calling abort() as is done elsewhere in the project.
| params->q_heads = calloc(params->n_queue, sizeof(cache_obj_t *)); | ||
| params->q_tails = calloc(params->n_queue, sizeof(cache_obj_t *)); | ||
| params->q_n_bytes = calloc(params->n_queue, sizeof(int64_t)); | ||
| params->q_n_objs = calloc(params->n_queue, sizeof(int64_t)); |
There was a problem hiding this comment.
The memory allocated by these calloc calls is not checked for NULL. An allocation failure would lead to a crash when the returned pointers are dereferenced. Please add NULL checks for each allocation and handle potential failures gracefully.
| static void MQ_parse_params(cache_t *cache, const char *cache_specific_params) { | ||
| MQ_params_t *params = (MQ_params_t *)cache->eviction_params; | ||
|
|
||
| char *params_str = strdup(cache_specific_params); |
There was a problem hiding this comment.
strdup can return NULL if memory allocation fails. This would cause a crash in the while loop condition or when strsep is called. Please add a NULL check for params_str after this line.
| cache_obj_t *obj = cache_insert_base(cache, req); | ||
|
|
||
| obj->misc.freq = 1; | ||
| obj->SLRU.lru_id = 0; |
There was a problem hiding this comment.
The SLRU.lru_id field is being reused to store the queue level for an object. While this appears to be a common pattern in this codebase to avoid modifying cache_obj_t, it's not immediately obvious. Adding a comment here would improve code clarity and maintainability for future developers.
| obj->SLRU.lru_id = 0; | |
| obj->SLRU.lru_id = 0; // Use SLRU field to store MQ level |
There was a problem hiding this comment.
Pull request overview
This PR adds a new frequency-aware Multi-Queue (MQ) eviction policy to libCacheSim and wires it into the library API and simulator frontend so it can be selected via CLI.
Changes:
- Implemented
MultiQueueeviction algorithm with configurablen-queue/n-queues/nqparameter. - Exposed
MultiQueue_initin the public eviction algorithm header and registered CLI aliases (mq,multi-queue,multiqueue). - Updated build system and documentation to include/list the new algorithm.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| libCacheSim/include/libCacheSim/evictionAlgo.h | Adds MultiQueue_init to the public eviction algorithm API. |
| libCacheSim/cache/eviction/MultiQueue.c | New MQ eviction implementation (multi-LRU queues with log-frequency promotion). |
| libCacheSim/cache/CMakeLists.txt | Adds MQ source file to the cache library build. |
| libCacheSim/bin/cachesim/cache_init.h | Registers MQ names/aliases for simulator cache creation. |
| README.md | Lists MultiQueue among supported eviction algorithms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| cache_t *MultiQueue_init(const common_cache_params_t ccache_params, | ||
| const char *cache_specific_params) { | ||
| cache_t *cache = | ||
| cache_struct_init("MultiQueue", ccache_params, cache_specific_params); | ||
| cache->cache_init = MultiQueue_init; | ||
| cache->cache_free = MQ_free; | ||
| cache->get = MQ_get; | ||
| cache->find = MQ_find; | ||
| cache->insert = MQ_insert; | ||
| cache->evict = MQ_evict; | ||
| cache->remove = MQ_remove; | ||
| cache->to_evict = MQ_to_evict; | ||
| cache->get_occupied_byte = cache_get_occupied_byte_default; | ||
| cache->can_insert = cache_can_insert_default; | ||
| cache->get_n_obj = cache_get_n_obj_default; | ||
|
|
||
| if (ccache_params.consider_obj_metadata) { | ||
| cache->obj_md_size = 8 * 2; | ||
| } else { | ||
| cache->obj_md_size = 0; | ||
| } | ||
|
|
||
| cache->eviction_params = malloc(sizeof(MQ_params_t)); | ||
| memset(cache->eviction_params, 0, sizeof(MQ_params_t)); | ||
| MQ_params_t *params = (MQ_params_t *)cache->eviction_params; | ||
|
|
||
| params->n_queue = 8; | ||
| MQ_parse_params(cache, DEFAULT_CACHE_PARAMS); | ||
| if (cache_specific_params != NULL) { | ||
| MQ_parse_params(cache, cache_specific_params); | ||
| } | ||
|
|
||
| params->q_heads = calloc(params->n_queue, sizeof(cache_obj_t *)); | ||
| params->q_tails = calloc(params->n_queue, sizeof(cache_obj_t *)); | ||
| params->q_n_bytes = calloc(params->n_queue, sizeof(int64_t)); | ||
| params->q_n_objs = calloc(params->n_queue, sizeof(int64_t)); | ||
|
|
||
| snprintf(cache->cache_name, CACHE_NAME_ARRAY_LEN, "MQ(%d)", params->n_queue); | ||
|
|
||
| return cache; | ||
| } |
| return obj; | ||
| } | ||
|
|
||
| obj->misc.freq += 1; |
| while (params_str != NULL && params_str[0] != '\0') { | ||
| char *key = strsep((char **)¶ms_str, "="); | ||
| char *value = strsep((char **)¶ms_str, ","); | ||
|
|
||
| if (strcasecmp(key, "n-queue") == 0 || strcasecmp(key, "n-queues") == 0 || | ||
| strcasecmp(key, "nq") == 0) { | ||
| params->n_queue = (int)strtol(value, NULL, 0); |
Motivation
Description
MultiQueueatlibCacheSim/cache/eviction/MultiQueue.cthat tracks objects in multiple LRU queues and promotes them by logarithmic-frequency levels, with a default of 8 queues and a parseablen-queue(n-queues/nq) parameter.MultiQueue_inittolibCacheSim/include/libCacheSim/evictionAlgo.h.mq,multi-queue,multiqueue) inlibCacheSim/bin/cachesim/cache_init.hso the algorithm can be selected by the simulator frontend.eviction/MultiQueue.ctolibCacheSim/cache/CMakeLists.txtand updatedREADME.mdto listMultiQueueamong supported eviction algorithms.Testing
rg/sedinspections) to verify new symbols, file additions, and registrations succeeded and found the new files referenced in header, CMake, and README (success).cmake -S . -B buildwhich failed due to a missing system dependency:Package 'glib-2.0', required by 'virtual:world', not found, so a full compile/test run could not be completed in this environment.Codex Task