Skip to content

Commit

Permalink
perf: Use Cow for value in MergedCSTNode (#64)
Browse files Browse the repository at this point in the history
In most scenarios, we don't need to create a new `String` when merging
the values of two CSTNodes. More often than not, we simply pick the
value from either the left or right node, or, ideally, the values of
both nodes are equal to the base. Consequently, creating owned Strings
is uncommon, and it would be more efficient to return a reference to the
value of one of the `CSTNodes`.

To accommodate this use case, this PR updates the `MergedCSTNode` to use
`Cow<str>` instead of `String`. This change eliminates unnecessary
`to_string` calls, potentially leading to performance improvements in
some scenarios.
  • Loading branch information
jpedroh authored Jul 27, 2024
1 parent 52fa6d1 commit c495920
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 105 deletions.
23 changes: 15 additions & 8 deletions bin/src/control.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{
error::Error,
fmt::{self, Display},
fmt::{self, Display}, time::Instant,
};

use matching::MatchingEntry;
Expand Down Expand Up @@ -58,33 +58,40 @@ pub fn run_tool_on_merge_scenario(

let parser_configuration = ParserConfiguration::from(language);

let start = Instant::now();
log::info!("Started parsing base file");
let base_tree =
parsing::parse_string(base, &parser_configuration).map_err(ExecutionError::ParsingError)?;
log::info!("Finished parsing base file");
log::info!("Finished parsing base file in {:?}", start.elapsed());

let start = Instant::now();
log::info!("Started parsing left file");
let left_tree =
parsing::parse_string(left, &parser_configuration).map_err(ExecutionError::ParsingError)?;
log::info!("Finished parsing left file");
log::info!("Finished parsing left file in {:?}", start.elapsed());

let start = Instant::now();
log::info!("Started parsing right file");
let right_tree = parsing::parse_string(right, &parser_configuration)
.map_err(ExecutionError::ParsingError)?;
log::info!("Finished parsing right file");
log::info!("Finished parsing right file in {:?}", start.elapsed());

let start = Instant::now();
log::info!("Started calculation of matchings between left and base");
let matchings_left_base = matching::calculate_matchings(&left_tree, &base_tree);
log::info!("Finished calculation of matchings between left and base");
log::info!("Finished calculation of matchings between left and base in {:?}", start.elapsed());

let start = Instant::now();
log::info!("Started calculation of matchings between right and base");
let matchings_right_base = matching::calculate_matchings(&right_tree, &base_tree);
log::info!("Finished calculation of matchings between right and base");
log::info!("Finished calculation of matchings between right and base in {:?}", start.elapsed());

let start = Instant::now();
log::info!("Started calculation of matchings between left and right");
let matchings_left_right = matching::calculate_matchings(&left_tree, &right_tree);
log::info!("Finished calculation of matchings between left and right");
log::info!("Finished calculation of matchings between left and right in {:?}", start.elapsed());

let start = Instant::now();
log::info!("Starting merge of the trees");
let result = merge::merge(
&base_tree,
Expand All @@ -95,7 +102,7 @@ pub fn run_tool_on_merge_scenario(
&matchings_left_right,
)
.map_err(ExecutionError::MergeError)?;
log::info!("Finished merge of the trees");
log::info!("Finished merge of the trees in {:?}", start.elapsed());

match result.has_conflict() {
true => Ok(ExecutionResult::WithConflicts(result.to_string())),
Expand Down
33 changes: 21 additions & 12 deletions merge/src/merge_terminals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ pub fn merge_terminals<'a>(
left: &'a Terminal<'a>,
right: &'a Terminal<'a>,
) -> Result<MergedCSTNode<'a>, MergeError> {
log::trace!("Calling merge terminal");

// Nodes of different kind, early return
if left.kind != right.kind {
return Err(MergeError::NodesWithDifferentKinds(
Expand All @@ -15,27 +17,34 @@ pub fn merge_terminals<'a>(
));
}

let left_equals_base = left.value == base.value;
let right_equals_base = right.value == base.value;

// Unchanged
if left.value == base.value && right.value == base.value {
Ok(base.to_owned().into())
if left_equals_base && right_equals_base {
log::trace!("Unchanged");
Ok(base.into())
// Changed in both
} else if left.value != base.value && right.value != base.value {
} else if !left_equals_base && !right_equals_base {
log::trace!("Changed in both");
match diffy::merge(base.value, left.value, right.value) {
Ok(value) => Ok(MergedCSTNode::Terminal {
kind: base.kind,
value,
value: std::borrow::Cow::Owned(value),
}),
Err(value) => Ok(MergedCSTNode::Terminal {
kind: base.kind,
value,
value: std::borrow::Cow::Owned(value),
}),
}
// Only left changed
} else if left.value != base.value {
Ok(left.to_owned().into())
} else if right_equals_base {
log::trace!("Only left changed");
Ok(left.into())
// Only right changed
} else {
Ok(right.to_owned().into())
log::trace!("Only right changed");
Ok(right.into())
}
}

Expand Down Expand Up @@ -75,7 +84,7 @@ mod tests {
&node,
&node,
&node,
&node.clone().into(),
&(&node).into(),
)
}

Expand Down Expand Up @@ -113,7 +122,7 @@ mod tests {
&right,
&MergedCSTNode::Terminal {
kind: "kind",
value: "left\nvalue\nright".to_string(),
value: std::borrow::Cow::Borrowed("left\nvalue\nright"),
},
)
}
Expand Down Expand Up @@ -150,7 +159,7 @@ mod tests {
merge_terminals(&base, &left, &right)?,
MergedCSTNode::Terminal {
kind: "kind",
value: "<<<<<<< ours\nleft_value||||||| original\nvalue=======\nright_value>>>>>>> theirs\n".to_string()
value: std::borrow::Cow::Borrowed("<<<<<<< ours\nleft_value||||||| original\nvalue=======\nright_value>>>>>>> theirs\n")
}
);

Expand Down Expand Up @@ -181,7 +190,7 @@ mod tests {
&base_and_left,
&base_and_left,
&changed_parent,
&changed_parent.clone().into(),
&(&changed_parent).into(),
)
}

Expand Down
19 changes: 8 additions & 11 deletions merge/src/merged_cst_node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use model::{
pub enum MergedCSTNode<'a> {
Terminal {
kind: &'a str,
value: String,
value: std::borrow::Cow<'a, str>,
},
NonTerminal {
kind: &'a str,
Expand All @@ -21,28 +21,25 @@ pub enum MergedCSTNode<'a> {
},
}

impl<'a> From<CSTNode<'a>> for MergedCSTNode<'a> {
fn from(val: CSTNode<'a>) -> Self {
impl<'a> From<&'a CSTNode<'a>> for MergedCSTNode<'a> {
fn from(val: &'a CSTNode<'a>) -> Self {
match val {
CSTNode::Terminal(Terminal { kind, value, .. }) => MergedCSTNode::Terminal {
kind,
value: value.to_string(),
},
CSTNode::Terminal(terminal) => terminal.into(),
CSTNode::NonTerminal(NonTerminal { kind, children, .. }) => {
MergedCSTNode::NonTerminal {
kind,
children: children.into_iter().map(|node| node.into()).collect(),
children: children.iter().map(|node| node.into()).collect(),
}
}
}
}
}

impl<'a> From<Terminal<'a>> for MergedCSTNode<'a> {
fn from(val: Terminal<'a>) -> Self {
impl<'a> From<&'a Terminal<'a>> for MergedCSTNode<'a> {
fn from(val: &'a Terminal<'a>) -> Self {
MergedCSTNode::Terminal {
kind: val.kind,
value: val.value.to_string(),
value: std::borrow::Cow::Borrowed(val.value),
}
}
}
Expand Down
Loading

0 comments on commit c495920

Please sign in to comment.