-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Improve JsonPatch diff performances #331
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some mostly syntactic suggestions and one remark about dependencies, but looks good overall 👍
.settings( | ||
name := "diffson-benchmarks", | ||
libraryDependencies ++= Seq( | ||
"io.circe" %% "circe-literal" % circeVersion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"io.circe" %% "circe-literal" % circeVersion | |
"io.circe" %% "circe-core" % circeVersion |
You don't seem to use literal
, just syntax from core
.
_.prepend(Remove(path / fld, if (rememberOld) Some(value1) else None))) | ||
} | ||
case Nil => | ||
Eval.now(Chain.fromSeq(fields2.toList).map { case (fld, value) => Add(path / fld, value) }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eval.now(Chain.fromSeq(fields2.toList).map { case (fld, value) => Add(path / fld, value) }) | |
Eval.now(Chain.fromSeq(fields2.toList.map { case (fld, value) => Add(path / fld, value) })) |
I'm pretty sure map
is slightly faster on List
. I also wonder whether one can go more directly from Map
to Chain
, but you probably tried already.
def remove(from: Int, until: Int, shift: Int, arr: List[Json]): Chain[Operation[Json]] = | ||
Chain.fromSeq( | ||
for (idx <- until to from by -1) | ||
yield Remove[Json](path / idx, if (rememberOld) Some(arr(idx - shift)) else None)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yield Remove[Json](path / idx, if (rememberOld) Some(arr(idx - shift)) else None)) | |
yield Remove[Json](path / idx, Option.when(rememberOld)(arr(idx - shift)))) |
case None => | ||
// field is not in the second object, delete it | ||
fieldsDiff(fields1, fields2, path).map( | ||
_.prepend(Remove(path / fld, if (rememberOld) Some(value1) else None))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_.prepend(Remove(path / fld, if (rememberOld) Some(value1) else None))) | |
_.prepend(Remove(path / fld, Option.when(rememberOld)(value1)))) |
The diffing algorithm was really slow in case of big collections and deeply nested structures.
The main reasons were:
Patience
LCS computationreverse_:::
)Chain
greatly improves collection appendsMoreover on deeply nested structures, the call stack was exploding. Since the diff methods are mutually recursive, tail recursion elimination was not easy to achieve. The new implementation switches to
Eval
internally to make the recursive calls stack safe.Below the results of the benchmarks for big arrays and deeply nested objects.
Before:
After: