Introduce Myers algorithm in linear space #112

lppedd · 2024-05-16T12:22:19Z

This PR requires #111 to be merged first.
I will then rebase.

codecov · 2024-05-16T12:27:59Z

Codecov Report

Attention: Patch coverage is 94.21769% with 17 lines in your changes missing coverage. Please review.

Project coverage is 86.07%. Comparing base (875f4ce) to head (37933be).

Files	Patch %	Lines
...github/petertrr/diffutils/text/DiffRowGenerator.kt	93.61%	3 Missing and 6 partials ⚠️
...futils/algorithm/myers/MyersDiffWithLinearSpace.kt	92.78%	1 Missing and 6 partials ⚠️
...hub/petertrr/diffutils/algorithm/myers/PathNode.kt	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #112      +/-   ##
============================================
+ Coverage     84.26%   86.07%   +1.80%     
- Complexity      151      221      +70     
============================================
  Files            22       30       +8     
  Lines           534      632      +98     
  Branches         89      117      +28     
============================================
+ Hits            450      544      +94     
- Misses           57       59       +2     
- Partials         27       29       +2

Flag	Coverage Δ
unittests	`86.07% <94.21%> (+1.80%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Required to correctly host the new code.

This fixes a long-standing bug, where we did not copy the list, but merely return of view of it.

This replaces the untyped lambda.

testPerformanceProblemsIssue124 showed a 30s vs 7s runtime. Moving to fields instead of getters drastically improved performance down to 10s. Still, not optimal.

lppedd · 2024-05-20T10:27:53Z

src/commonMain/kotlin/io/github/petertrr/diffutils/text/DiffRowGenerator.kt

+ val origList = inlineDiffSplitter.split(joinedOrig)
+ val revList = inlineDiffSplitter.split(joinedRev)
+
+ // Copying of `origList` and `revList` is needed because otherwise `wrapInTag` results in ConcurrentModificationException


@petertrr I don't understand why this would throw a ConcurrentModificationException

I'm not sure TBH; this part was copied from the original library.

@petertrr no problem, I can remove it, it's not needed as we aren't operating on the lists concurrently.

lppedd · 2024-05-20T10:29:40Z

I'm done with this one. The refactor: clean up DiffRowGenerator commit has a bit of changes but they're mostly to remove "untyped" lambdas.

This PR is a breaking API change for DiffRowGenerator.

lppedd · 2024-05-20T11:40:26Z

.gitignore

+.DS_Store
+.kotlin/


Kotlin 2.0 introduces a new folder to store metadata.

lppedd · 2024-05-20T11:40:55Z

README.md


+- fuzzy patches


Will port in another PR.

petertrr

Hey, sorry for disappearing. Your PR generally LGTM, I have just a couple of questions. Also, what's the breaking change in the DiffRowGenerator? Is it a new constructor parameter, or did I overlook something else?

petertrr · 2024-05-19T18:25:00Z

src/commonMain/kotlin/io/github/petertrr/diffutils/algorithm/Change.kt

@@ -19,11 +19,12 @@
 package io.github.petertrr.diffutils.algorithm

 import io.github.petertrr.diffutils.patch.DeltaType
+import kotlin.jvm.JvmField

 public data class Change(


I wonder if making the class @JvmRecord instead would have similar effect

@petertrr we are targeting Java 8, so it wouldn't make any difference unfortunately.
But anyway, with records the only difference is method naming, e.g. getX() > x(), so you'd still not expose fields directly.

petertrr · 2024-06-04T20:29:49Z

src/commonMain/kotlin/io/github/petertrr/diffutils/algorithm/myers/MyersDiffWithLinearSpace.kt

+
+ while (i < end1 || j < end2) {
+ if (i < end1 && j < end2 && equalizer.test(data.source[i], data.target[j])) {
+ // script.append(new KeepCommand<>(left.charAt(i)));


What's the plan for the commented code?

@petertrr I'll get rid of them!

petertrr · 2024-06-04T20:32:18Z

src/commonMain/kotlin/io/github/petertrr/diffutils/text/DiffRowGenerator.kt

+ val origList = inlineDiffSplitter.split(joinedOrig)
+ val revList = inlineDiffSplitter.split(joinedRev)
+
+ // Copying of `origList` and `revList` is needed because otherwise `wrapInTag` results in ConcurrentModificationException


I'm not sure TBH; this part was copied from the original library.

lppedd · 2024-06-04T21:01:16Z

what's the breaking change in the DiffRowGenerator

It's just the constructor, we introduced new interfaces so I'm not sure it would work straight away.
Edit: well, the new interfaces are not only there, so I'd say this is a general breaking change, but, guess it's ok!

petertrr

LGTM, thanks for a nice improvement!

lppedd · 2024-06-11T21:27:50Z

@petertrr np. I have another PR with a small cleanup and then it would be cool to do a release imo!

lppedd · 2024-06-11T21:32:18Z

Ah I see you already released ahaha, well no problem, will be for another minor release.

lppedd marked this pull request as ready for review May 17, 2024 09:40

lppedd added 12 commits May 20, 2024 00:01

chore: disable unnecessary Detekt rules

53153e2

Required to correctly host the new code.

fix: replace subList with slice

034774e

This fixes a long-standing bug, where we did not copy the list, but merely return of view of it.

refactor: introduce the DiffEqualizer interface

b07b979

This replaces the untyped lambda.

refactor: rename Chunk.verify to Chunk.verifyChunk

cbb1f4f

refactor: align delta's string representation with java-diff

3a87497

feat: introduce MyersDiffWithLinearSpace

df9a059

feat: allow skipping delta decompression

ef11cb1

perf: mitigate *embarrassing* performance difference with java-diff

69e32f6

testPerformanceProblemsIssue124 showed a 30s vs 7s runtime. Moving to fields instead of getters drastically improved performance down to 10s. Still, not optimal.

chore: refresh yarn.lock

241a30d

refactor: clean up DiffRowGenerator

84029fd

build: prepare for Kotlin 2.0

743edaa

docs: update readme

f3d1caf

lppedd force-pushed the refactor/myers-linear-space branch from 45d28ee to f3d1caf Compare May 20, 2024 10:24

lppedd commented May 20, 2024

View reviewed changes

.gitignore

.DS_Store

.kotlin/

Copy link

Contributor Author

lppedd May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kotlin 2.0 introduces a new folder to store metadata.

petertrr reacted with thumbs up emoji

lppedd commented May 20, 2024

View reviewed changes

README.md

- fuzzy patches

Copy link

Contributor Author

lppedd May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will port in another PR.

petertrr reviewed Jun 4, 2024

View reviewed changes

lppedd added 2 commits June 5, 2024 10:00

refactor: remove commented out lines

0121c61

refactor: remove unnecessary list copying

37933be

lppedd requested a review from petertrr June 5, 2024 08:11

petertrr approved these changes Jun 11, 2024

View reviewed changes

petertrr merged commit edfab24 into petertrr:main Jun 11, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce Myers algorithm in linear space #112

Introduce Myers algorithm in linear space #112

lppedd commented May 16, 2024

codecov bot commented May 16, 2024 •

edited

lppedd May 20, 2024

petertrr Jun 4, 2024

lppedd Jun 4, 2024

lppedd commented May 20, 2024

lppedd May 20, 2024

lppedd May 20, 2024

petertrr left a comment

petertrr May 19, 2024

lppedd Jun 4, 2024

petertrr Jun 4, 2024

lppedd Jun 4, 2024

petertrr Jun 4, 2024

lppedd commented Jun 4, 2024 •

edited

petertrr left a comment

lppedd commented Jun 11, 2024

lppedd commented Jun 11, 2024

Introduce Myers algorithm in linear space #112

Introduce Myers algorithm in linear space #112

Conversation

lppedd commented May 16, 2024

codecov bot commented May 16, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lppedd commented May 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petertrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lppedd commented Jun 4, 2024 • edited

petertrr left a comment

Choose a reason for hiding this comment

lppedd commented Jun 11, 2024

lppedd commented Jun 11, 2024

codecov bot commented May 16, 2024 •

edited

lppedd commented Jun 4, 2024 •

edited