-
Notifications
You must be signed in to change notification settings - Fork 1
Bindings for cache admission policies #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @AlexSutila, 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 enhances the Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces Python bindings for cache admission policies, a valuable addition that enhances the library's flexibility. The implementation includes bindings for existing policies and a PluginAdmissioner
for custom logic, complete with examples. While the overall approach is sound, I've identified a couple of critical memory management issues in the C++ bindings that must be addressed. Additionally, there are several suggestions for the Python code to improve type safety, robustness, and adherence to conventions. Addressing these points will significantly improve the quality and reliability of this new feature.
static void pypluginAdmissioner_free(admissioner_t *admissioner) { | ||
pypluginAdmissioner_params_t *params = | ||
(pypluginAdmissioner_params_t *)admissioner->params; | ||
params->admissioner_free_hook(params->data); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pypluginAdmissioner_free
function calls the Python free_hook
but fails to deallocate the pypluginAdmissioner_params_t
struct, which was allocated using new
. This results in a memory leak every time a PluginAdmissioner
is destroyed. The implementation should be updated to ensure proper cleanup by using a std::unique_ptr
with the custom deleter, similar to the pattern in pypluginCache_free
.
static void pypluginAdmissioner_free(admissioner_t *admissioner) {
if (!admissioner || !admissioner->params) {
return;
}
// Take ownership to ensure proper cleanup via the custom deleter.
std::unique_ptr<pypluginAdmissioner_params_t, PypluginAdmissionerParamsDeleter> params_deleter(
static_cast<pypluginAdmissioner_params_t *>(admissioner->params));
admissioner->params = nullptr;
}
|
||
|
||
def admit_hook(data, request): | ||
admit = random.randint(1, 10) == 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add tests and documentation? |
Yes, I will try to do so later tonight if I am able to find the time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR. Looks good to me overall, just a few comments.
Understood, I'm in agreement with all suggestions mentioned above. I was able to write some admissioner tests this morning, and will be sure to mention the extra optional arguments when I add to the documentation. I'll try to push something out addressing all the above soon. Thank you. |
I have just pushed a few more commits which do the following:
Some notes on the docs, I felt writing about the admission bindings for existing admission algorithms under the Cache Simulation page made the most sense. Since this page was initially blank, I also tried to document some of the existing replacement policies as well before showing documentation for admissioners. Final note on the docs, similar to PluginCache, I wrote the documentation for PluginAdmissioner under the Plugin System page, just because it felt appropriate. Ofc, if you feel anything I added to the docs is best placed somewhere else I have no issues moving any of it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! LGTM.
I will leave it to @haochengxia to merge it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive Python bindings for cache admission policies in libCacheSim, enabling users to apply admission policies to existing cache algorithms without modifying the underlying C++ backend. The main purpose is to provide both bindings for existing admission policies (BloomFilter, Prob, Size, SizeProbabilistic, AdaptSize) and a plugin system for custom admission policies.
Key changes include:
- Bindings for existing admission policies with configurable parameters
- A
PluginAdmissioner
system allowing custom admission policies via Python hooks - Integration of admission policies into all cache classes through an optional
admissioner
parameter
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
src/export_admissioner.cpp |
Core C++ bindings for admission policies and plugin system implementation |
libcachesim/admissioner.py |
Python wrapper classes for admission policies including the plugin system |
libcachesim/cache.py |
Integration of admission policies into all cache classes via optional parameter |
tests/test_admission.py |
Comprehensive test suite for all admission policy types |
examples/admission/ |
Example usage of both existing and custom admission policies |
docs/src/en/examples/ |
Documentation updates for admission policies and plugin system |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
LGTM. Thank you! |
This PR adds the following features, without requiring any additional changes to the underlying libCacheSim backend:
BloomFilterAdmissioner
,ProbAdmissioner
,SizeAdmissioner
,SizeProbabilisticAdmissioner
, andAdaptSizeAdmissioner
. Arguments that would otherwise be passed through cli can now be configured optionally through the constructors of these classes. Example below:bloomfilter.py
lol):A
PluginAdmissioner
which is similar to thePluginCache
in nature, allowing custom admission policies to be defined easily through hooks to each function pointer instruct admissioner_t
.I've included an additional
admission
directory underexamples
showing how to use existing policies and also thePluginAdmissioner
with a toy example that randomly admits 10% of incoming requests and tracks the admit rate, displaying it in the free hook as another sanity check. From what I've seen the admit rate has always been reasonably close.Lastly, I feel it is still worth mentioning that providing an admissioner at all is still optional and the library should continue to work as it did otherwise if no admissioner is provided.