[WIP] Use multiple CELPluginInstances #324
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request implements instance pooling for CEL plugins, allowing multiple instances to be created and reused to improve performance and concurrency handling.
- Introduces a
CELPluginInstancePoolwith LRU cache and TTL-based expiration for managing plugin instances - Refactors the service generation to use plugin references instead of single instances
- Adds context-based instance management with proper cleanup mechanisms
Reviewed Changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| grpc/federation/cel/plugin.go | Main implementation of instance pooling with LRU cache and context management |
| go.mod | Adds hashicorp/golang-lru/v2 dependency for LRU cache functionality |
| generator/templates/server.go.tmpl | Updates template to use plugin references and context-based instance management |
| _examples/18_load/proto/plugin/plugin.proto | Adds network capability configuration to example |
| _examples/18_load/plugin/plugin_grpc_federation.pb.go | Generated code with network transport initialization |
| _examples/18_load/plugin/plugin.pb.go | Updated protobuf generated code |
| _examples/18_load/main_test.go | Test fixes for goroutine leak detection |
| _examples/18_load/go.mod | Dependency updates for example |
| _examples/18_load/federation/federation_grpc_federation.pb.go | Generated federation service code with new pooling |
| _examples/18_load/compose.yaml | Docker compose configuration for testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if cache := getCompilationCache(cfg.Name, cfg.CacheDir); cache != nil { | ||
| runtimeCfg = runtimeCfg.WithCompilationCache(cache) | ||
| } | ||
| } else { | ||
| runtimeCfg = wazero.NewRuntimeConfig() | ||
| if cache := getCompilationCache(cfg.Name, cfg.CacheDir); cache != nil { | ||
| runtimeCfg = runtimeCfg.WithCompilationCache(cache) | ||
| } | ||
| } |
There was a problem hiding this comment.
The compilation cache is being set when cfg.CacheDir is empty, but getCompilationCache will return nil when baseDir (cfg.CacheDir) is empty and no temp directory is available. This creates inconsistent behavior between the two branches.
| if cache := getCompilationCache(cfg.Name, cfg.CacheDir); cache != nil { | |
| runtimeCfg = runtimeCfg.WithCompilationCache(cache) | |
| } | |
| } else { | |
| runtimeCfg = wazero.NewRuntimeConfig() | |
| if cache := getCompilationCache(cfg.Name, cfg.CacheDir); cache != nil { | |
| runtimeCfg = runtimeCfg.WithCompilationCache(cache) | |
| } | |
| } | |
| } else { | |
| runtimeCfg = wazero.NewRuntimeConfig() | |
| } | |
| if cache := getCompilationCache(cfg.Name, cfg.CacheDir); cache != nil { | |
| runtimeCfg = runtimeCfg.WithCompilationCache(cache) | |
| } |
Copilot uses AI. Check for mistakes.
| pool.cache = expirable.NewLRU(1, func(_ *CELPluginInstance, v *CELPluginInstance) { | ||
| if _, exists := pool.guardMap.Load(v); exists { | ||
| return | ||
| } | ||
| v.Close() | ||
| }, 1*time.Minute) |
There was a problem hiding this comment.
[nitpick] The magic number 1 for initial LRU size and 1*time.Minute for TTL should be configurable constants. Hard-coding these values makes the pooling behavior difficult to tune for different use cases.
Copilot uses AI. Check for mistakes.
| if noCap := p.cache.Add(v, v); noCap { | ||
| // grow cache capacity. | ||
| p.cache.Resize(p.cache.Len() * 2) | ||
| p.cache.Add(v, v) | ||
| //fmt.Println("grow length", p.cache.Len()) | ||
| } |
There was a problem hiding this comment.
[nitpick] The cache growth strategy using p.cache.Len() * 2 could lead to unbounded memory usage. Consider implementing a maximum cache size limit to prevent excessive memory consumption.
Copilot uses AI. Check for mistakes.
| _, removed, ok := p.cache.RemoveOldest() | ||
| p.guardMap.Delete(oldest) | ||
| if oldest == removed && ok { | ||
| //fmt.Printf("get: %p\n", removed) |
There was a problem hiding this comment.
Remove commented debug print statements from production code. These should be removed or converted to proper logging if needed for debugging.
Copilot uses AI. Check for mistakes.
| } | ||
| } | ||
| ret, err := p.fn() | ||
| //fmt.Printf("new: %p\n", ret) |
There was a problem hiding this comment.
Remove commented debug print statements from production code. These should be removed or converted to proper logging if needed for debugging.
Copilot uses AI. Check for mistakes.
| // grow cache capacity. | ||
| p.cache.Resize(p.cache.Len() * 2) | ||
| p.cache.Add(v, v) | ||
| //fmt.Println("grow length", p.cache.Len()) |
There was a problem hiding this comment.
Remove commented debug print statements from production code. These should be removed or converted to proper logging if needed for debugging.
| //fmt.Println("grow length", p.cache.Len()) |
Copilot uses AI. Check for mistakes.
| func (p *CELPlugin) ReleaseInstance(instance *CELPluginInstance) { | ||
| instance.enqueueGC() | ||
| p.instancePool.Put(instance) | ||
| //fmt.Printf("put: %p\n", instance) |
There was a problem hiding this comment.
Remove commented debug print statements from production code. These should be removed or converted to proper logging if needed for debugging.
| //fmt.Printf("put: %p\n", instance) |
Copilot uses AI. Check for mistakes.
No description provided.