perf(luavm): cache compiled Lua FunctionProto in RunScript to avoid r…#7602
perf(luavm): cache compiled Lua FunctionProto in RunScript to avoid r…#7602qsxDree wants to merge 4 commits into
Conversation
karmada-bot
added
the
kind/feature
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 optimizes the Lua VM execution path by implementing a caching layer for compiled scripts. By storing and reusing FunctionProto objects, the system avoids the overhead of repeated parsing and compilation, which significantly improves performance and reduces memory pressure in high-load scenarios where scripts are executed frequently. 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 the 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 counterproductive. 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. Footnotes
|
|
Welcome @kurlingtown! It looks like this is your first PR to karmada-io/karmada 🎉 |
karmada-bot
added
the
size/L
![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 compilation caching for Lua scripts within the VM by caching compiled FunctionProto structures in a sync.Map, which avoids redundant compilation during script execution. Unit tests were also added to verify this caching behavior. The reviewer feedback recommends adding a TODO comment to implement an eviction policy for the cache to prevent potential memory leaks, and adding a documentation comment to the unexported compileScript helper function to improve code maintainability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // UseOpenLibs flag to enable open libraries. Libraries are disabled by default while running, but enabled during testing to allow the use of print statements. | ||
| UseOpenLibs bool | ||
| Pool *fixedpool.FixedPool | ||
| funcCache sync.Map // map[string]*lua.FunctionProto |
There was a problem hiding this comment.
The funcCache uses a sync.Map to cache compiled Lua FunctionProto structures. Since scripts can be updated or dynamically registered, this cache can grow indefinitely over time, leading to a potential memory leak. Please add a TODO comment indicating that a garbage collection or eviction policy should be implemented.
// TODO(user): Implement a garbage collection or eviction policy for funcCache to prevent memory leaks if scripts are updated or dynamically generated.
funcCache sync.Map // map[string]*lua.FunctionProtoReferences
- When implementing garbage collection (GC) for caches, ensure the GC method is invoked periodically (e.g., in a background goroutine) to prevent memory leaks. If the invocation is deferred, use a TODO comment to prevent confusion for other developers.
| return vm | ||
| } | ||
|
|
||
| func (vm *VM) compileScript(script string) (*lua.FunctionProto, error) { |
There was a problem hiding this comment.
The compileScript function is an unexported helper containing key compilation and caching logic. According to the general rules, unexported functions with key logic should have doc comments to improve maintainability.
// compileScript parses and compiles the Lua script into a FunctionProto, caching the result to avoid redundant compilation.
func (vm *VM) compileScript(script string) (*lua.FunctionProto, error) {References
- Add doc comments to unexported functions that contain key logic to improve maintainability.
…edundant parse+compile Signed-off-by: kurlingtown <kurling.town@gmail.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7602 +/- ##
==========================================
- Coverage 42.16% 42.06% -0.10%
==========================================
Files 879 879
Lines 54731 54855 +124
==========================================
+ Hits 23076 23077 +1
- Misses 29911 30029 +118
- Partials 1744 1749 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Signed-off-by: kurlingtown <kurling.town@gmail.com>
|
/cc @RainbowMango |
|
Putting this into my queue. Thanks. |
|
@qsxDree Yes, I like the approach. But I think the memory leak concerns should be addressed, as you mentioned on the comments:
|
zhzhuang-zju
left a comment
zhzhuang-zju
left a comment
There was a problem hiding this comment.
Hi @qsxDree, this change brings performance improvements. Please add a corresponding release note in the PR description, thanks.
| } | ||
|
|
||
| // compileScript parses and compiles the Lua script into a FunctionProto, caching the result to avoid redundant compilation. | ||
| func (vm *VM) compileScript(script string) (*lua.FunctionProto, error) { |
There was a problem hiding this comment.
How about:
| func (vm *VM) compileScript(script string) (*lua.FunctionProto, error) { | |
| func (vm *VM) loadOrCompileScript(script string) (*lua.FunctionProto, error) { |
There was a problem hiding this comment.
yea that makes more sense. updated!
to address the memory leak concern, i could implement the functionality of clearing the |
done. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Do you mean let configManager refresh the cache in LuaVM? I'm not sure, but another idea in my mind is that, taking group/version/kind, and operation type as the cache key, the cache will be re-compiled once the script changes. |
sorry for my inexperience, but i don't get how having group/version/kind, and operation type as the cache key will enable to detect change in lua script. like none of those factors will change on modification of the script right? |
Yes, you are right. But note that each script belongs to a Group/Version/Kind AND operation. The cache workflow might like:
|
okay, you mean have the gvk + operation type as key and the {string script, compiled proto} as the value, right? PS: |
Signed-off-by: qsxDree <kurling.town@gmail.com>
Signed-off-by: qsxDree <kurling.town@gmail.com>
Yes, that's the idea.
Looks great, but one more thing needs to be confirmed: is the script index constant? |
the index is the position in the list for the current config snapshot. so it is constant for an unchanged config but not if it is updated. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Cache compiled Lua
FunctionProtoin the VM to avoid redundant parse+compile on everyRunScriptcall.RunScriptis called on every status event throughReflectStatus,InterpretHealth,AggregateStatus, etc. In large fleets this causes continuous unnecessary allocation and GC pressure. Since scripts are immutable, recompiling every time is pure waste.Fixes #
This fix caches compiled protos using
sync.Mapkeyed by script content and reuses viaNewFunctionFromProto+PCall. Behavior remains identical.Which issue(s) this PR fixes:
Fixes #7597
Part of #7596
Does this PR introduce a user-facing change?: