perf(core): avoid config clones in prefix-filtered resolution#1091
perf(core): avoid config clones in prefix-filtered resolution#1091sauraww wants to merge 1 commit into
Conversation
|
Review changes with Changed Files
|
Walkthrough
ChangesPrefix-filtered resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Improves superposition_core configuration resolution performance for prefix-filtered requests by removing the full Config deep-clone and instead applying prefix filtering while collecting override keys and filtering the already-owned default config directly. Adds benchmark coverage and documents the before/after results for the prefix-filtered path.
Changes:
- Update
eval_config/eval_config_with_reasoningto resolve against borrowedcontexts/overridesfor both unfiltered and prefix-filtered requests. - Apply prefix filtering inside
get_overrides(skip non-matching override keys) and filter default config keys before merging. - Expand the Criterion benchmark and performance analysis doc with prefix-filtered results and comparison against the previous cloned approach.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/misc/PERF_ANALYSIS_RESOLUTION.md | Updates the perf write-up to reflect the new borrowed prefix-filter path and adds prefix benchmark results. |
| crates/superposition_core/src/config.rs | Refactors prefix-filtered resolution to avoid cloning full config; adds prefix-filter helpers and threads prefix filtering into override collection. |
| crates/superposition_core/benches/resolve.rs | Adds prefix-filtered benchmark variants comparing pre-optimization cloned behavior vs optimized borrowed behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let filter_prefixes: Option<HashSet<String>> = filter_prefixes | ||
| .filter(|prefixes| !prefixes.is_empty()) | ||
| .map(HashSet::from_iter); |
| match merge_strategy { | ||
| MergeStrategy::REPLACE => { | ||
| for (key, value) in overriden_value.iter() { | ||
| if !matches_prefix_filter(key, prefix_filter) { | ||
| continue; | ||
| } | ||
| required_overrides.insert(key.clone(), value.clone()); | ||
| } | ||
| } | ||
| MergeStrategy::MERGE => { | ||
| for (key, value) in overriden_value.iter() { | ||
| if !matches_prefix_filter(key, prefix_filter) { | ||
| continue; | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/superposition_core/src/config.rs (1)
22-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
eval_configandeval_config_with_reasoningare now identical — extract shared logic.After this refactor, both functions have byte-for-byte identical bodies (same
filter_prefixesconversion,get_overridescall withNonecallback,filter_config_keys_by_prefix, andmerge_overrides_on_default_config). Any future bug fix or logic change must be applied in both places. Sinceeval_config_with_reasoningno longer adds reasoning metadata (confirmed by the test at line 210), it can simply delegate toeval_config.♻️ Proposed delegation
pub fn eval_config_with_reasoning( default_config: Map<String, Value>, contexts: &[Context], overrides: &HashMap<String, Overrides>, dimensions: &HashMap<String, DimensionInfo>, query_data: &Map<String, Value>, merge_strategy: MergeStrategy, filter_prefixes: Option<Vec<String>, // Optional prefix filtering ) -> Result<Map<String, Value>, String> { - let modified_query_data = evaluate_local_cohorts(dimensions, query_data); - - let filter_prefixes: Option<HashSet<String>> = filter_prefixes - .filter(|prefixes| !prefixes.is_empty()) - .map(HashSet::from_iter); - - let overrides_map = get_overrides( - &modified_query_data, - contexts, - overrides, - &merge_strategy, - filter_prefixes.as_ref(), - None, - )?; - - let mut result_config = match &filter_prefixes { - Some(prefixes) => filter_config_keys_by_prefix(default_config, prefixes), - None => default_config, - }; - merge_overrides_on_default_config(&mut result_config, overrides_map, &merge_strategy); - - Ok(result_config) + eval_config( + default_config, + contexts, + overrides, + dimensions, + query_data, + merge_strategy, + filter_prefixes, + ) }Also applies to: 55-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/superposition_core/src/config.rs` around lines 22 - 41, Extract the duplicated implementation from eval_config_with_reasoning and make it delegate directly to eval_config, preserving the existing parameters and return behavior. Remove the repeated filter_prefixes conversion, get_overrides call, config filtering, and merge logic so these operations remain maintained only in eval_config.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/superposition_core/src/config.rs`:
- Around line 22-41: Extract the duplicated implementation from
eval_config_with_reasoning and make it delegate directly to eval_config,
preserving the existing parameters and return behavior. Remove the repeated
filter_prefixes conversion, get_overrides call, config filtering, and merge
logic so these operations remain maintained only in eval_config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 04d172dc-6302-40a9-93c8-6877414bdced
📒 Files selected for processing (3)
crates/superposition_core/benches/resolve.rscrates/superposition_core/src/config.rsdocs/misc/PERF_ANALYSIS_RESOLUTION.md
Problem
Prefix-filtered configuration resolution deep-cloned the complete configuration, including all contexts, overrides, and dimensions, before applying the prefix filter. With the synthetic production-scale dataset of 468,006 contexts, this added significant allocation and cloning overhead to every prefix-filtered resolve.
Solution
Updated
eval_configandeval_config_with_reasoningto use borrowed contexts and overrides for prefix-filtered resolution. Prefix filtering is now applied while collecting override keys, while the already-owned default configuration is filtered directly.The benchmark now compares the previous cloned prefix path with the optimized borrowed path:
904.50 ms55.535 ms16.3xfaster, or a93.9%reductionThe benchmark uses 468,006 synthetic contexts, 18 dimensions, 18 local cohorts, and 100 rotating queries. Detailed results are documented in
docs/misc/PERF_ANALYSIS_RESOLUTION.md.Environment variable changes
None.
BENCH_CONTEXTSremains an optional benchmark-only variable for overriding the default synthetic context count.Pre-deployment activity
None.
Post-deployment activity
None.
API changes
No API or endpoint changes.
Possible Issues in the future
Resolution still performs a linear scan over all contexts, and prefix matching checks each configured prefix against relevant keys. Large context datasets or many prefixes may therefore remain expensive.
The benchmark measures the core resolution path using synthetic data. It does not include provider/SDK overhead, network latency, cache initialization, or end-to-end P99 latency. Future improvements could include evaluation caching or indexing contexts to reduce the number of rules scanned per resolve.
Summary by CodeRabbit
Performance Improvements
Documentation