Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal: replace difference with similar #51

Closed

Conversation

tommilligan
Copy link
Collaborator

@tommilligan tommilligan commented Feb 11, 2021

See #44

Major rewrite to remove the dependency on difference, which is unmaintained.

@codecov-io
Copy link

Codecov Report

Merging #51 (bbeb51f) into main (f02609f) will increase coverage by 2.87%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
+ Coverage   96.55%   99.42%   +2.87%     
==========================================
  Files           5        5              
  Lines         145      174      +29     
==========================================
+ Hits          140      173      +33     
+ Misses          5        1       -4     
Impacted Files Coverage Δ
tests/assert_ne.rs 96.96% <ø> (ø)
tests/pretty_string.rs 100.00% <ø> (ø)
src/lib.rs 100.00% <100.00%> (ø)
src/printer.rs 100.00% <100.00%> (ø)
tests/assert_eq.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f02609f...bbeb51f. Read the comment docs.

@tommilligan
Copy link
Collaborator Author

@mitsuhiko I've benchmarked this branch vs main using the benchmarks here, unfortunately there appear to be some performance regressions for writing the diff:

image

I'm going to continue investigating, but this means I'm unlikely to merge this implementation as is.

@tommilligan
Copy link
Collaborator Author

Closing for now as obsolete from #52, happy to revisit if the performance can be improved.

@mitsuhiko
Copy link

Thanks for the heads up. I will add some benchmarks to figure out what’s going on.

@mitsuhiko
Copy link

mitsuhiko commented Feb 14, 2021

@tommilligan I'm not sure yet why myers performs so bad but I added a simple LCS implementation to see how it performs. I noticed two things from this:

  • iter_all_changes internally needs to call Box::new for stupid lifetime reasons I can't figure out. You get a small performance benefit by calling ops + iter_changes yourself.
  • next version of similar comes with Lcs you can use like this:
    let diff = TextDiff::configure()
        .algorithm(Algorithm::Lcs)
        .diff_lines(left, right);

Both of those together are similar in performance to the current main version of rust-pretty-assertions. Sadly I'm on a laptop so I can't do stable benchmarks here. It does say it outperforms diff but I can't be sure.

I also swapped out Myer's for a better implementation now that performs significantly better.

@mitsuhiko
Copy link

Pushed out a 1.2.0 with the changes. And thanks for the box fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants