Various improvements to performance and stability #36
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Signed-off-by: John Howard <howardjohn@google.com>
Signed-off-by: John Howard <howardjohn@google.com>
Signed-off-by: John Howard <howardjohn@google.com>
Signed-off-by: John Howard <howardjohn@google.com>
Signed-off-by: John Howard <howardjohn@google.com>
Signed-off-by: John Howard <howardjohn@google.com>
Signed-off-by: John Howard <howardjohn@google.com>
Signed-off-by: John Howard <howardjohn@google.com>
``` name old time/op new time/op delta CreatePatch/complex-48 167µs ± 8% 156µs ± 4% -6.85% (p=0.001 n=10+10) CreatePatch/large_array-48 664ms ± 1% 2ms ± 3% -99.71% (p=0.000 n=10+10) CreatePatch/simple-48 2.95µs ± 2% 2.92µs ± 1% ~ (p=0.447 n=10+10) name old alloc/op new alloc/op delta CreatePatch/complex-48 75.8kB ± 0% 75.0kB ± 0% -0.95% (p=0.000 n=10+10) CreatePatch/large_array-48 153MB ± 0% 1MB ± 0% -99.39% (p=0.000 n=9+10) CreatePatch/simple-48 1.23kB ± 0% 1.23kB ± 0% +0.04% (p=0.033 n=10+10) name old allocs/op new allocs/op delta CreatePatch/complex-48 1.20k ± 0% 1.17k ± 0% -2.41% (p=0.000 n=10+10) CreatePatch/large_array-48 7.01M ± 0% 0.01M ± 0% -99.79% (p=0.000 n=10+10) CreatePatch/simple-48 29.0 ± 0% 29.0 ± 0% ~ (all equal) ``` Signed-off-by: John Howard <howardjohn@google.com>
Signed-off-by: John Howard <howardjohn@google.com>
howardjohn
force-pushed
the
v2/improvements
branch
from
4ebc697
to
6d5c3df
Compare
This was referenced May 16, 2023
|
Please LMK what you think @tamalsaha . Thanks! |
tamalsaha
reviewed
May 22, 2023
| @@ -149,9 +155,6 @@ func makePath(path string, newPart interface{}) string { | |||
| if path == "" { | |||
| return "/" + key | |||
| } | |||
| if strings.HasSuffix(path, "/") { | |||
There was a problem hiding this comment.
The test case for this is:
emptyKeyA = `{"":[0]}`
emptyKeyB = `{"":[]}`
The patch should be
[
{
"op": "remove",
"path": "//0"
}
]
Before this change, we would have the path /0. which was not correct.
I am not sure why it was originally added. Removal of this code did not impact any existing test cases, and the fuzz tester should verify we round-trip successfully. So I think the only case this code was hit was for the empty key edge case
|
One question, otherwise LGTM. |
tamalsaha
approved these changes
May 22, 2023
|
Tagged https://github.com/gomodules/jsonpatch/releases/tag/v2.3.0 Thanks a lot for your contribution! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces a series of patches with the ultimate goal of improving performance. In our use case, we see this library as bottleneck - applying single patches can take >30s and allocate GBs of data against relatively small objects (Kubernetes objects size, not like GBs of json or anything crazy).
In the process of fixing this, I improved the test coverage by adding fuzz tests and benchmarks to ensure the changes I made actually improved performance, and did not cause regressions.
As part of this, the fuzz tests found a few bugs around edge case handling which are also fixed:
"") handlingFinally, the compareEditDistance optimization is removed. This is what brings a (massive) performance improvement, while still passing all round-tripping tests (including the new fuzz tests, which gives a high degree of confidence). The function as written introduces n^2 behavior, behaving poorly on large array inputs. Simply removing this appears to pass all tests, so I took that approach.
Results:
This patch is applied to the v2 branch only, rather than v3, as the usage of ordermap in the v3 branch is a blocker for us due to similar performance concerns. If desired, I can port the test/bugfix improvements to v3 as well, although we would not personally use it.\
Fuzzing results: