Conversation
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
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 significantly expands the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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 several new compute-aware cache eviction algorithms, including BeladyCompute, LHD_compute, S3FIFOCompute, and GDSF_compute. The changes are extensive and add significant new functionality. However, the review identified a critical issue with potential infinite recursion in BeladyCompute, several high-severity out-of-bounds memory access risks due to missing checks before accessing request features, and some medium-severity issues related to thread-safety and logical inconsistencies. These issues should be addressed to ensure the stability and correctness of the new algorithms.
| (unsigned long long)cache->get_n_obj(cache), | ||
| (long long)cache->cache_size, (long long)req->obj_size, | ||
| params->n_sample, obj_to_evict_score, sampled_obj_score); | ||
| return BeladyCompute_to_evict(cache, req); |
There was a problem hiding this comment.
The recursive call to BeladyCompute_to_evict(cache, req) when obj_to_evict is NULL can lead to infinite recursion and a stack overflow. This can happen if the cache is empty or if random sampling consistently fails to find a suitable eviction candidate. You should add a base case to prevent this, for example by using an iterative approach or limiting the recursion depth.
| tag->timestamp = timestamp; | ||
| tag->app = DEFAULT_APP_ID % APP_CLASSES; | ||
| tag->size = 1; | ||
| tag->compute_intensity = req->features[0]; |
There was a problem hiding this comment.
The code directly accesses req->features[0] without checking if req->n_features > 0. If a request arrives with no features, this will result in an out-of-bounds memory access, which is undefined behavior and can lead to a crash. You should add a check before accessing the feature.
tag->compute_intensity = (req->n_features > 0) ? req->features[0] : 1;| // Higher obj_size -> higher ratio -> harder to promote | ||
| // Modified: ratio = mean_obj_size_in_small / req_compute_intensity | ||
| // Higher compute_intensity -> lower ratio -> easier to promote | ||
| double compute_intensity = req->features[0]; |
There was a problem hiding this comment.
The code directly accesses req->features[0] without checking if req->n_features > 0. If a request arrives with no features, this will result in an out-of-bounds memory access, which is undefined behavior and can lead to a crash. This issue is present in multiple places in this file. You should add a check before accessing the feature.
double compute_intensity = (req->n_features > 0) ? req->features[0] : 1.0;| cache_obj_t *obj = cache_insert_base(cache, req); | ||
| DEBUG_ASSERT(obj != nullptr); | ||
| obj->misc.freq = 1; | ||
| obj->GDSF_compute.compute_intensity = req->features[0]; |
There was a problem hiding this comment.
The code directly accesses req->features[0] without checking if req->n_features > 0. If a request arrives with no features, this will result in an out-of-bounds memory access, which is undefined behavior and can lead to a crash. You should add a check before accessing the feature.
obj->GDSF_compute.compute_intensity = (req->n_features > 0) ? req->features[0] : 1;| #ifdef RECORD_EVICTION_PROCESS | ||
| FILE *eviction_process_ofile = NULL; | ||
|
|
||
| void set_new_record_eviction_process_file(const char *ofilepath) { | ||
| if (eviction_process_ofile != NULL) { | ||
| fclose(eviction_process_ofile); | ||
| } | ||
| eviction_process_ofile = fopen(ofilepath, "w"); | ||
| if (eviction_process_ofile == NULL) { | ||
| ERROR("cannot open eviction process file %s\n", ofilepath); | ||
| } | ||
| } | ||
|
|
||
| void print_eviction_debug_message(const char *msg) { | ||
| if (eviction_process_ofile != NULL) { | ||
| fprintf(eviction_process_ofile, "%s\n", msg); | ||
| } | ||
| } | ||
| #endif /* RECORD_EVICTION_PROCESS */ |
There was a problem hiding this comment.
The RECORD_EVICTION_PROCESS feature uses a global file pointer eviction_process_ofile. This is not thread-safe. If multiple threads use the cache simulator simultaneously, they will race on this global variable, leading to corrupted output or crashes. Consider making this thread-local using __thread or protecting access with a mutex if multi-threaded use is intended.
| if (req->n_features > 0) { | ||
| obj->Belady.compute_intensity = req->features[0]; | ||
| } |
There was a problem hiding this comment.
In BeladyCompute_find, when req->n_features <= 0, the compute_intensity of the object is not updated. This is inconsistent with BeladyCompute_insert, which sets a default value of 1 in this case. An object might retain a stale compute_intensity from a previous access. It would be more robust to also set a default value here.
if (req->n_features > 0) {
obj->Belady.compute_intensity = req->features[0];
} else {
obj->Belady.compute_intensity = 1;
}| obj->misc.freq = 1; | ||
| obj->GDSF_compute.compute_intensity = req->features[0]; | ||
|
|
||
| double pri = gdsf->pri_last_evict + req->features[0]; |
There was a problem hiding this comment.
The priority calculation for a new object on insert seems inconsistent with the priority update on a cache hit. On a hit (in GDSF_compute_find), the priority is updated using sqrt(compute_intensity). However, on insert, the priority is calculated using compute_intensity directly. For consistency, you should probably use sqrt here as well.
double pri = gdsf->pri_last_evict + sqrt((double)obj->GDSF_compute.compute_intensity);There was a problem hiding this comment.
Pull request overview
This PR adds compute-intensity-aware variants of several cache eviction algorithms (Belady, S3FIFO, GDSF, LHD) to libCacheSim. These variants use req->features[0] as a compute intensity signal to bias eviction decisions toward keeping compute-intensive objects cached longer.
Changes:
- New eviction algorithms:
BeladyCompute,S3FIFOCompute,S3FIFOSize,GDSF_compute,LHD_compute - Debug infrastructure for recording eviction processes (
RECORD_EVICTION_PROCESS) - Registration of some (but not all) new algorithms in the cachesim binary
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
evictionAlgo.h |
Declares init functions for new algorithms |
cacheObj.h |
Adds compute_intensity fields to cache object metadata |
cache.h |
Adds optional eviction process recording API |
cache.c |
Implements eviction recording; logs evicted obj IDs |
BeladyCompute.c |
Belady variant using reuse_distance/compute_intensity scoring |
S3FIFOSize.c |
Size-aware S3-FIFO variant |
S3FIFOCompute.c |
Compute-intensity-aware S3-FIFO variant |
GDSF_compute.cpp |
GDSF variant incorporating compute intensity into priority |
lhd_compute.hpp/cpp |
LHD variant scaling hit density by compute intensity |
LHD_compute_interface.cpp |
C interface for LHD compute variant |
CMakeLists.txt |
Registers new source files for build |
cache_init.h |
Registers lhd_compute and beladyCompute in cachesim CLI |
💡 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.
| if (compute_intensity <= 0) compute_intensity = 1.0; // Avoid division by zero | ||
| double ratio = mean_obj_size_in_small / compute_intensity; | ||
|
|
||
| cache_obj_t *ghost_obj = ghost_q->find(ghost_q, req, false); |
| // MODIFIED: Apply compute intensity logic | ||
| double compute_intensity = req->features[0]; | ||
| if (compute_intensity <= 0) compute_intensity = 1.0; // Avoid division by zero |
| // MODIFIED: Apply compute intensity logic | ||
| double compute_intensity = req->features[0]; |
| #endif | ||
| return obj; | ||
| } | ||
|
|
||
| obj = params->main->find(params->main, req, true); | ||
| if (obj != NULL) { | ||
| obj->S3FIFO.freq += 1; |
| obj = params->ghost->find(params->ghost, req, false); | ||
| if (obj == NULL) { | ||
| obj = params->ghost->insert(params->ghost, req); | ||
| obj->S3FIFO.freq = 1; | ||
| } else { | ||
| obj->S3FIFO.freq += 1; |
| obj = params->ghost->find(params->ghost, req, false); | ||
| if (obj == NULL) { | ||
| obj = params->ghost->insert(params->ghost, req); | ||
| obj->S3FIFO.freq = 1; | ||
| } else { | ||
| obj->S3FIFO.freq += 1; |
|
|
||
| double ratio = (double)req->obj_size / mean_obj_size_in_small; | ||
|
|
||
| cache_obj_t *ghost_obj = ghost_q->find(ghost_q, req, false); |
|
|
||
| char *params_str = strdup(cache_specific_params); | ||
| char *old_params_str = params_str; | ||
| char *end; |
| cache_evict_base(cache, obj_to_evict, true); | ||
| } | ||
|
|
||
| bool BeladyCompute_remove(cache_t *cache, const obj_id_t obj_id) { |
No description provided.