From e404c0e86fe9e183db8cf067e205e6bcb360c9a1 Mon Sep 17 00:00:00 2001 From: Wilfred Hughes Date: Fri, 14 Jan 2022 09:47:40 -0800 Subject: [PATCH] Ensure we only calculate opposite positions once Previously this was computed per-hunk, which was wasteful and slow when there are many hunks. --- CHANGELOG.md | 7 +++++- src/context.rs | 22 +++++++++-------- src/hunks.rs | 60 +++++++++++++++++++++++++++++++-------------- src/inline.rs | 9 ++++--- src/main.rs | 9 +++++-- src/side_by_side.rs | 9 ++++--- 6 files changed, 79 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67684d3df..2915b364c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,12 @@ Improved language detection for files with bash/sh syntax. ### Integration -Fixed a crash when on Mercurial diffs when a whole file has been removed. +Fixed a crash when on Mercurial diffs when a whole file has been +removed. + +### Display + +Improved display performance when there are a large number of hunks. ## 0.15 (released 6 January 2022) diff --git a/src/context.rs b/src/context.rs index b3377f567..3c554f95c 100644 --- a/src/context.rs +++ b/src/context.rs @@ -150,7 +150,7 @@ pub fn flip_tuples(items: &[(Tx, Ty)]) -> Vec<(Ty, Tx)> { /// 122 91 (closest match) fn after_with_opposites( after_lines: &[LineNumber], - opposite_lines: HashMap>, + opposite_lines: &HashMap>, prev_max_opposite: Option, max_opposite: LineNumber, ) -> Vec<(Option, Option)> { @@ -196,17 +196,14 @@ fn after_with_opposites( pub fn calculate_context( lines: &[(Option, Option)], - lhs_mps: &[MatchedPos], - rhs_mps: &[MatchedPos], + opposite_to_lhs: &HashMap>, + opposite_to_rhs: &HashMap>, max_lhs_src_line: LineNumber, max_rhs_src_line: LineNumber, ) -> ( Vec<(Option, Option)>, Vec<(Option, Option)>, ) { - let opposite_to_lhs = opposite_positions(lhs_mps); - let opposite_to_rhs = opposite_positions(rhs_mps); - let before_lines: Vec<_> = match lines.first() { Some(first_line) => match *first_line { (Some(lhs_line), _) => { @@ -266,13 +263,18 @@ pub fn calculate_context( pub fn add_context( lines: &[(Option, Option)], - lhs_mps: &[MatchedPos], - rhs_mps: &[MatchedPos], + opposite_to_lhs: &HashMap>, + opposite_to_rhs: &HashMap>, max_lhs_src_line: LineNumber, max_rhs_src_line: LineNumber, ) -> Vec<(Option, Option)> { - let (before_lines, after_lines) = - calculate_context(lines, lhs_mps, rhs_mps, max_lhs_src_line, max_rhs_src_line); + let (before_lines, after_lines) = calculate_context( + lines, + opposite_to_lhs, + opposite_to_rhs, + max_lhs_src_line, + max_rhs_src_line, + ); before_lines .into_iter() diff --git a/src/hunks.rs b/src/hunks.rs index caa9879a0..5374f1c55 100644 --- a/src/hunks.rs +++ b/src/hunks.rs @@ -11,7 +11,7 @@ use std::{ }; use crate::{ - context::{add_context, calculate_context, flip_tuples, opposite_positions}, + context::{add_context, calculate_context, flip_tuples}, lines::LineNumber, syntax::{zip_pad_shorter, MatchedPos}, }; @@ -127,8 +127,8 @@ fn extract_lines(hunk: &Hunk) -> Vec<(Option, Option)> { pub fn merge_adjacent( hunks: &[Hunk], - lhs_mps: &[MatchedPos], - rhs_mps: &[MatchedPos], + opposite_to_lhs: &HashMap>, + opposite_to_rhs: &HashMap>, max_lhs_src_line: LineNumber, max_rhs_src_line: LineNumber, ) -> Vec { @@ -143,8 +143,13 @@ pub fn merge_adjacent( let mut rhs_lines: HashSet = HashSet::new(); let lines = extract_lines(hunk); - let contextual_lines = - add_context(&lines, lhs_mps, rhs_mps, max_lhs_src_line, max_rhs_src_line); + let contextual_lines = add_context( + &lines, + opposite_to_lhs, + opposite_to_rhs, + max_lhs_src_line, + max_rhs_src_line, + ); for (lhs_line, rhs_line) in contextual_lines { if let Some(lhs_line) = lhs_line { lhs_lines.insert(lhs_line); @@ -672,8 +677,8 @@ fn fill_matched_lines( pub fn aligned_lines_from_hunk( hunk: &Hunk, - lhs_mps: &[MatchedPos], - rhs_mps: &[MatchedPos], + opposite_to_lhs: &HashMap>, + opposite_to_rhs: &HashMap>, max_lhs_src_line: LineNumber, max_rhs_src_line: LineNumber, ) -> Vec<(Option, Option)> { @@ -681,8 +686,8 @@ pub fn aligned_lines_from_hunk( let (before_context, after_context) = calculate_context( &hunk_lines, - lhs_mps, - rhs_mps, + opposite_to_lhs, + opposite_to_rhs, max_lhs_src_line, max_rhs_src_line, ); @@ -693,12 +698,9 @@ pub fn aligned_lines_from_hunk( let mut res: Vec<(Option, Option)> = vec![]; res.extend(before_context); - let matched_lhs_lines = opposite_positions(rhs_mps); - let matched_rhs_lines = opposite_positions(lhs_mps); - if let (Some(start_pair), Some(end_pair)) = (start_pair, end_pair) { // Fill lines between. - let aligned_between = fill_aligned(start_pair, end_pair, &matched_rhs_lines); + let aligned_between = fill_aligned(start_pair, end_pair, &opposite_to_rhs); // TODO: align based on blank lines too. @@ -711,13 +713,13 @@ pub fn aligned_lines_from_hunk( let aligned_between = match first_novel { (Some(lhs_start), _) => { - fill_matched_lines(lhs_start, max_lhs_src_line, hunk_end, &matched_rhs_lines) + fill_matched_lines(lhs_start, max_lhs_src_line, hunk_end, opposite_to_lhs) } (_, Some(rhs_start)) => flip_tuples(&fill_matched_lines( rhs_start, max_rhs_src_line, hunk_end, - &matched_lhs_lines, + opposite_to_rhs, )), (None, None) => unreachable!(), }; @@ -735,7 +737,7 @@ mod tests { use super::*; use crate::{ positions::SingleLineSpan, - syntax::{AtomKind, MatchKind, TokenKind}, + syntax::{AtomKind, MatchKind, TokenKind}, context::opposite_positions, }; use pretty_assertions::assert_eq; @@ -892,7 +894,18 @@ mod tests { }, ]; - let res = aligned_lines_from_hunk(&hunk, &lhs_mps, &rhs_mps, 1.into(), 2.into()); + // TODO: Just build a HashMap diretly instead of positions + // first for this test. + let opposite_to_lhs = opposite_positions(&lhs_mps); + let opposite_to_rhs = opposite_positions(&rhs_mps); + + let res = aligned_lines_from_hunk( + &hunk, + &opposite_to_lhs, + &opposite_to_rhs, + 1.into(), + 2.into(), + ); assert_eq!( res, vec![ @@ -1028,7 +1041,18 @@ mod tests { }, ]; - let res = aligned_lines_from_hunk(&hunk, &lhs_mps, &rhs_mps, 1.into(), 3.into()); + // TODO: Just build a HashMap diretly instead of positions + // first for this test. + let opposite_to_lhs = opposite_positions(&lhs_mps); + let opposite_to_rhs = opposite_positions(&rhs_mps); + + let res = aligned_lines_from_hunk( + &hunk, + &opposite_to_lhs, + &opposite_to_rhs, + 1.into(), + 3.into(), + ); assert_eq!( res, vec![ diff --git a/src/inline.rs b/src/inline.rs index 2c5b8d2ea..0a7c21ac4 100644 --- a/src/inline.rs +++ b/src/inline.rs @@ -1,7 +1,7 @@ //! Inline, or "unified" diff display. use crate::{ - context::calculate_context, + context::{calculate_context, opposite_positions}, hunks::Hunk, lines::{format_line_num, MaxLine}, style::apply_colors, @@ -24,12 +24,15 @@ pub fn display( let mut res = String::new(); + let opposite_to_lhs = opposite_positions(lhs_positions); + let opposite_to_rhs = opposite_positions(rhs_positions); + for hunk in hunks { let hunk_lines = hunk.lines.clone(); let (before_lines, after_lines) = calculate_context( &hunk_lines, - lhs_positions, - rhs_positions, + &opposite_to_lhs, + &opposite_to_rhs, // TODO: repeatedly calculating the maximum is wasteful. lhs_src.max_line(), rhs_src.max_line(), diff --git a/src/main.rs b/src/main.rs index cb2030c61..73c57a40b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -24,6 +24,7 @@ mod tree_sitter_parser; extern crate log; use crate::hunks::{matched_pos_to_hunks, merge_adjacent}; +use context::opposite_positions; use files::read_files_or_die; use guess_language::guess; use log::info; @@ -338,11 +339,15 @@ fn print_diff_result(summary: &DiffResult) { return; } + let opposite_to_lhs = opposite_positions(&summary.lhs_positions); + let opposite_to_rhs = opposite_positions(&summary.rhs_positions); + + let hunks = matched_pos_to_hunks(&summary.lhs_positions, &summary.rhs_positions); let hunks = merge_adjacent( &hunks, - &summary.lhs_positions, - &summary.rhs_positions, + &opposite_to_lhs, + &opposite_to_rhs, summary.lhs_src.max_line(), summary.rhs_src.max_line(), ); diff --git a/src/side_by_side.rs b/src/side_by_side.rs index 6eaa42bf7..1200efa22 100644 --- a/src/side_by_side.rs +++ b/src/side_by_side.rs @@ -14,7 +14,7 @@ use crate::{ lines::{codepoint_len, format_line_num, LineNumber, MaxLine}, positions::SingleLineSpan, style::{self, apply_colors, color_positions, split_and_apply, Style}, - syntax::{zip_pad_shorter, MatchedPos}, + syntax::{zip_pad_shorter, MatchedPos}, context::opposite_positions, }; const SPACER: &str = " "; @@ -250,6 +250,9 @@ pub fn display_hunks( lhs_mps: &[MatchedPos], rhs_mps: &[MatchedPos], ) -> String { + let opposite_to_lhs = opposite_positions(lhs_mps); + let opposite_to_rhs = opposite_positions(rhs_mps); + let lhs_colored_src = apply_colors(lhs_src, true, lhs_mps); let rhs_colored_src = apply_colors(rhs_src, false, rhs_mps); @@ -284,8 +287,8 @@ pub fn display_hunks( let aligned_lines = aligned_lines_from_hunk( hunk, - lhs_mps, - rhs_mps, + &opposite_to_lhs, + &opposite_to_rhs, lhs_src.max_line(), rhs_src.max_line(), );