search

Palindrom / JSONPatcherProxy

ES6 proxy powered JSON Object observer that emits JSON patches when changes occur to your object tree.
94 stars 14 forks source link

Make sure collected patches are not mutated (continuation) #48

Closed warpech closed 5 years ago

warpech commented 5 years ago

Continuation of https://github.com/Palindrom/JSONPatcherProxy/pull/46 with test and performance improvement.

cc @eriksunsol

There is a serious degradation in performance (see "Complex mutation"), but I think it is an important fix. @tomalec WDYT?

Performance before this PR (npm run bench)

Observe and generate, small object (JSONPatcherProxy)
 Ops/sec: 78007 ±4.53% Ran 5595 times in 0.072 seconds.
Observe and generate (JSONPatcherProxy)
 Ops/sec: 1909 ±2.49% Ran 250 times in 0.131 seconds.
Primitive mutation (JSONPatcherProxy)
 Ops/sec: 504531 ±0.92% Ran 25937 times in 0.051 seconds.
Complex mutation (JSONPatcherProxy)
 Ops/sec: 12758 ±0.28% Ran 651 times in 0.051 seconds.
Serialization (JSONPatcherProxy)
 Ops/sec: 2089 ±0.47% Ran 107 times in 0.051 seconds.

Performance after this PR (npm run bench)

Observe and generate, small object (JSONPatcherProxy)
 Ops/sec: 78382 ±3.97% Ran 5371 times in 0.069 seconds.
Observe and generate (JSONPatcherProxy)
 Ops/sec: 1902 ±2.26% Ran 230 times in 0.121 seconds.
Primitive mutation (JSONPatcherProxy)
 Ops/sec: 521039 ±0.74% Ran 26589 times in 0.051 seconds.
Complex mutation (JSONPatcherProxy)
 Ops/sec: 2201 ±0.21% Ran 112 times in 0.051 seconds.
Serialization (JSONPatcherProxy)
 Ops/sec: 2106 ±0.18% Ran 107 times in 0.051 seconds.
warpech commented 5 years ago

Clever idea, however our current usage in Palindrom would require to parse the stringified JSON for sanity-checking: https://github.com/Palindrom/Palindrom/blob/e7442d6946cce9afb4b7e8c270b84287d110af96/src/palindrom.js#L211

However, a way to fix that: provide a validation function from Palindrom to JSONPatcherProxy, so that JSONPatcherProxy can validate the patch before generating it.

There is also some (~30%) pentalty in calling JSON.serialize separately for each operaion, as opposed to calling it once for the patch array (as we do now): http://jsbench.github.io/#2cd4ee42cc5997ddd05b7f348ab218d0

warpech commented 5 years ago

We discussed today with @tomalec and @eriksunsol, that this change is not worth the performance penalty. Rather than fixing the code in this library, we could write a warning in README.md that patch immutability is not a feature on this library. If you use the patch asynchrously, you ought to apply immutability on the patch object in your code. In our case, Palindrom libary would apply immutability on the patch before inserting it to the queue.

warpech commented 5 years ago

Closing in favor of https://github.com/Palindrom/JSONPatcherProxy/pull/55