refactor: remove unnecessary clone from eval logic#857
refactor: remove unnecessary clone from eval logic#857ayushjain17 wants to merge 1 commit intomainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR refactors ownership semantics across the codebase, shifting evaluation and configuration functions from borrowing references to accepting owned values. Public API signatures are updated for eval_config, eval_config_with_reasoning, filter_by_dimensions, get_applicable_variants, and local cohort evaluation functions. Internal implementations reduce cloning and use in-place mutations, with corresponding updates across FFI boundaries, call sites, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
ayushjain17
changed the title
ayushjain17
force-pushed
the
refactor/cloning
branch
from
26389f0 to
965c903
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/superposition_core/src/ffi_legacy.rs (1)
1-1:⚠️ Potential issue | 🟡 MinorStale file-level comment:
// src/ffi.rsdoesn't match filenameffi_legacy.rs.Proposed fix
-// src/ffi.rs +// src/ffi_legacy.rs
🤖 Fix all issues with AI agents
In `@crates/superposition_types/src/config.rs`:
- Around line 292-317: In filter_by_dimensions, remove the unnecessary clone of
self.contexts and pass ownership directly to Context::filter_by_eval (i.e., call
Context::filter_by_eval(self.contexts, &modified_context)) since
filter_by_dimensions consumes self and contexts is not used afterward; update
the call site in the function where filtered_context is created (referencing
Context::filter_by_eval and self.contexts) so you avoid the clone and retain the
existing use of self.overrides via let mut overrides = self.overrides.
🧹 Nitpick comments (4)
crates/superposition_provider/src/utils.rs (1)
654-670: Remaining clones inside the function body could be eliminated.Since
config(and thusfinal_config) is now owned, you can avoid the.clone()calls on lines 655 and 658 by destructuring the owned struct:♻️ Suggested refactor
- // Start with default configs - let mut result = final_config.default_configs.clone(); - - // Apply overrides based on context priority (higher priority wins) - let mut sorted_contexts = final_config.contexts.clone(); - sorted_contexts.sort_by_key(|c| std::cmp::Reverse(c.priority)); // Sort by priority descending + let Config { default_configs: mut result, mut contexts: sorted_contexts, overrides, .. } = final_config; + sorted_contexts.sort_by_key(|c| std::cmp::Reverse(c.priority)); for context in sorted_contexts { if let Some(override_key) = context.override_with_keys.first() { - if let Some(overrides) = final_config.overrides.get(override_key) { - let override_map: Map<String, Value> = overrides.clone().into(); + if let Some(overrides) = overrides.get(override_key) { + let override_map: Map<String, Value> = overrides.clone().into(); for (override_key, value) in override_map { result.insert(override_key, value);crates/superposition_provider/src/client.rs (1)
217-246: Remove unnecessary clone ofdefault_configson line 227.The
eval_configfunction signature incrates/superposition_core/src/config.rstakesdefault_configs: Map<String, Value>by value. Sincecached_configis already an ownedConfig(fromto_owned()on line 223) and the field is moved immediately without reuse, the.clone()is unnecessary. This matches the pattern used for the other fields (contexts,overrides,dimensions) which are moved without cloning.Suggested fix
eval_config( - cached_config.default_configs.clone(), + cached_config.default_configs, cached_config.contexts, cached_config.overrides, cached_config.dimensions,crates/superposition_core/src/config.rs (1)
144-163:overrides.remove()is a nice touch to avoid cloning override values.One minor observation: in the
REPLACEbranch (line 147),on_override_select(context)is guarded by theif let Some(doc)check, while in theMERGEbranch (line 159) it's unconditional. Sincerequired_overridesis always initialized asjson!({}),as_object_mut()will always succeed, so in practice the behavior is identical. However, for clarity and consistency you could consider moving theon_override_selectcall outside theif letin the REPLACE arm — or restructuring so both branches look symmetric.♻️ Optional: make both branches symmetric
MergeStrategy::REPLACE => { if let Some(doc) = required_overrides.as_object_mut() { for (key, value) in overriden_value.into_inner() { doc.insert(key, value); } - on_override_select(context) } + on_override_select(context); }crates/superposition_core/src/ffi.rs (1)
143-144: Unnecessaryquery_data.clone()—query_datais not used after this line.Since
json_from_maptakes ownership andquery_datahas no further uses, you can pass it directly.♻️ Remove unnecessary clone
- let _query_data = json_from_map(query_data.clone()) + let _query_data = json_from_map(query_data)
| pub fn filter_by_dimensions(self, dimension_data: Map<String, Value>) -> Self { | ||
| let modified_context = evaluate_local_cohorts_skip_unresolved( | ||
| self.dimensions.clone(), | ||
| dimension_data, | ||
| ); | ||
|
|
||
| let filtered_context = | ||
| Context::filter_by_eval(self.contexts.clone(), &modified_context); | ||
|
|
||
| let mut overrides = self.overrides; | ||
| let filtered_overrides: HashMap<String, Overrides> = filtered_context | ||
| .iter() | ||
| .flat_map(|ele| { | ||
| let override_with_key = ele.override_with_keys.get_key(); | ||
| self.overrides | ||
| .get(override_with_key) | ||
| .map(|value| (override_with_key.to_string(), value.clone())) | ||
| overrides | ||
| .remove(override_with_key) | ||
| .map(|value| (override_with_key.to_string(), value)) | ||
| }) | ||
| .collect(); | ||
|
|
||
| Self { | ||
| contexts: filtered_context, | ||
| overrides: filtered_overrides, | ||
| default_configs: self.default_configs.clone(), | ||
| dimensions: self.dimensions.clone(), | ||
| default_configs: self.default_configs, | ||
| dimensions: self.dimensions, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'fn filter_by_eval' --type rust -A 5Repository: juspay/superposition
Length of output: 1026
Remove unnecessary clone of self.contexts.
The filter_by_eval method accepts Vec<Self> by value (not by reference), so self.contexts can be passed directly without cloning:
let filtered_context = Context::filter_by_eval(self.contexts, &modified_context);Since self is consumed and self.contexts is not used afterward, the clone is avoidable.
🤖 Prompt for AI Agents
In `@crates/superposition_types/src/config.rs` around lines 292 - 317, In
filter_by_dimensions, remove the unnecessary clone of self.contexts and pass
ownership directly to Context::filter_by_eval (i.e., call
Context::filter_by_eval(self.contexts, &modified_context)) since
filter_by_dimensions consumes self and contexts is not used afterward; update
the call site in the function where filtered_context is created (referencing
Context::filter_by_eval and self.contexts) so you avoid the clone and retain the
existing use of self.overrides via let mut overrides = self.overrides.
965c903 to
f7210b4
Compare
ayushjain17
force-pushed
the
refactor/cloning
branch
from
f7210b4 to
20a244b
Compare
1 participant
Problem
Too much unnecessary cloning of data in eval logic
Solution
Take ownership of data in functions wherever necessary instead of borrowing and then cloning inside the functions, allowing the caller functions to decide upon necessary cloning and ownership transfer
Summary by CodeRabbit
Release Notes
Breaking Changes
Refactoring
New Features