-
Notifications
You must be signed in to change notification settings - Fork 14
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
Nice things (test refactor) from PR 39 #42
Merged
Merged
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
use a one liner to clone the object where possible
Warning, this commit changes the performance of the benchmark. Generation of tested object is now 2x faster. This is good for the benchmark, because cheaper object generation allows the benchmark to be more sensitive in catching the differences in the performance of JSONPatcherProxy. Just remember that the results from before and after this commit are not comparable. Benchmark results before this commit ============================= jsonpatcherproxy generate operation Ops/sec: 132404 ±2.55% Ran 8276 times in 0.063 seconds. jsonpatch generate operation Ops/sec: 106585 ±6.73% Ran 6434 times in 0.060 seconds. jsonpatcherproxy mutation - huge object Ops/sec: 906 ±2.45% Ran 51 times in 0.056 seconds. jsonpatch mutation - huge object Ops/sec: 862 ±1.52% Ran 47 times in 0.055 seconds. jsonpatcherproxy generate operation - huge object Ops/sec: 811 ±1.99% Ran 46 times in 0.057 seconds. jsonpatch generate operation - huge object Ops/sec: 847 ±1.20% Ran 47 times in 0.055 seconds. PROXIFY big object Ops/sec: 1813 ±2.00% Ran 102 times in 0.056 seconds. DIRTY-OBSERVE big object Ops/sec: 1625 ±1.48% Ran 91 times in 0.056 seconds. Benchmark results after this commit ============================= jsonpatcherproxy generate operation Ops/sec: 136915 ±2.46% Ran 8566 times in 0.063 seconds. jsonpatch generate operation Ops/sec: 104113 ±7.19% Ran 6330 times in 0.061 seconds. jsonpatcherproxy mutation - huge object Ops/sec: 1960 ±3.53% Ran 130 times in 0.066 seconds. jsonpatch mutation - huge object Ops/sec: 1607 ±1.84% Ran 88 times in 0.055 seconds. jsonpatcherproxy generate operation - huge object Ops/sec: 1562 ±3.40% Ran 101 times in 0.065 seconds. jsonpatch generate operation - huge object Ops/sec: 1620 ±2.04% Ran 90 times in 0.056 seconds. PROXIFY big object Ops/sec: 3968 ±4.07% Ran 294 times in 0.074 seconds. DIRTY-OBSERVE big object Ops/sec: 3285 ±1.83% Ran 180 times in 0.055 seconds.
the benchmark methods are almost exactly the same, but I have made the following changes: - group benchmarks by the topic - give more meaningful titles - wherever the preparation code is not meaningful for the benchmark, move it outside the benchmark - use code blocks to give scope to preparation variables this is a change that makes the benchmark results incomparable with the previous versions. however, it will work if you copy this file onto an older version and run it.
as requested in #39 (comment) the benchmark simply serializes the whole object, which triggers the "get" trap on every subtree
by default such comparison will be excluded. Benefits of this: - faster execution - shorter report to copy to PRs
to make functionality equal to fast-json-patch'es
This was referenced Jun 24, 2019
tomalec
approved these changes
Jun 24, 2019
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.
LGTM, thanks ❤️
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.
PR #39 contains discussable changes that downgrade performance, but also refactoring of the test suite.
Since I don't want to merge performance degradation until absolutely required, I decided to make another PR with things that are easier to agree with.
In fact, these changes were already agreed, because PR #39 got a green light for merging. Just in case, @tomalec please give a quick review again. Changes are only in
package.json
and tests.Changes are explained in the commit messages, but here's a short summary:
npm run bench
does not compare withfast-json-patch
anymorenpm run bench-compare
compares withfast-json-patch
and no-opI think this can be merged to
master
before PRs #39 and #41, then I can open new rebased PRs for #39 and #41 (this won't take long).Benchmark formatting before this PR:
Benchmark formatting after this PR: