Compatibility with external deep proxies

warpech commented 5 years ago

Fixes #35 through refactoring of the metadata access layer and adding a new check to the trap of "set".

Benchmark results

Before this PR

jsonpatcherproxy generate operation
 Ops/sec: 95630 ±4.20% Ran 7111 times in 0.074 seconds.
jsonpatch generate operation
 Ops/sec: 158564 ±4.14% Ran 9171 times in 0.058 seconds.
jsonpatcherproxy mutation - huge object
 Ops/sec: 839 ±4.87% Ran 56 times in 0.067 seconds.
jsonpatch mutation - huge object
 Ops/sec: 989 ±2.64% Ran 55 times in 0.056 seconds.
jsonpatcherproxy generate operation - huge object
 Ops/sec: 869 ±4.87% Ran 56 times in 0.064 seconds.
jsonpatch generate operation - huge object
 Ops/sec: 980 ±2.79% Ran 55 times in 0.056 seconds.
PROXIFY big object
 Ops/sec: 1671 ±4.91% Ran 109 times in 0.065 seconds.
DIRTY-OBSERVE big object
 Ops/sec: 1945 ±2.70% Ran 109 times in 0.056 seconds.

After this PR

jsonpatcherproxy generate operation
 Ops/sec: 89850 ±4.54% Ran 6729 times in 0.075 seconds.
jsonpatch generate operation
 Ops/sec: 155485 ±3.85% Ran 9117 times in 0.059 seconds.
jsonpatcherproxy mutation - huge object
 Ops/sec: 832 ±5.00% Ran 57 times in 0.069 seconds.
jsonpatch mutation - huge object
 Ops/sec: 988 ±2.60% Ran 55 times in 0.056 seconds.
jsonpatcherproxy generate operation - huge object
 Ops/sec: 807 ±5.01% Ran 53 times in 0.066 seconds.
jsonpatch generate operation - huge object
 Ops/sec: 979 ±2.74% Ran 55 times in 0.056 seconds.
PROXIFY big object
 Ops/sec: 1666 ±5.03% Ran 111 times in 0.067 seconds.
DIRTY-OBSERVE big object
 Ops/sec: 1932 ±3.28% Ran 109 times in 0.056 seconds.

tomalec commented 5 years ago

Could you add a test for creating external deep proxy? I still don't see a clear reason why previous implementation with a map is not capable of doing so. What's the difference between of having instance specific map than instance specific symbol?

warpech commented 5 years ago

Could you add a test for creating external deep proxy?

There is a test in the commit c461b492a4fa2e2869ba99cbe0329db3846e93cd

I still don't see a clear reason why previous implementation with a map is not capable of doing so. What's the difference between of having instance specific map than instance specific symbol?

Consider this. There are two ways of accessing an object:

by reference. In JSONPatcherProxy, such way of accessing is used whenever you see tree. This way gives you original unproxified instance of the object, because that's how the set trap works.
by parent and key. In JSONPatcherProxy, such way of accessing is used whenever you see tree[key]. This way gives you whatever the external world sees as the value, including the proxy made by the current instance of JSONPatcherProxy and any other proxy.

All places in JSONPatcherProxy where there was a need to use tree[key], used to use tree[key] as the key to a Map object. However, this only worked as long as the value at tree[key] was exactly the proxy object created by the current instance of JSONPatcherProxy. It the value was additionaly proxified by external code, it was detected by JSONPatcherProxy as a totally new object.

This is because in the ES Proxy world, the proxy instances have different identity than the original object:

var obj = {};
var p1 = new Proxy(obj, {});
var p2 = new Proxy(p1, {});
obj === p1; //false
p1 === p2; //false

The method used in this PR works by recognizing the object already proxified by an instance of JSONPatcherProxy not by keeping a reference to it, but by marking it from the inside:

var obj = {};
var meta = {};
var symb = Symbol("Symbol for getting the tree metadata from external access.");
obj[symb] = meta;

var p1 = new Proxy(obj, {});
var p2 = new Proxy(p1, {});
obj[symb] === p1[symb]; //true
p1[symb] === p2[symb]; //true

tomalec commented 5 years ago

There is a test in the commit c461b49

Sorry my bad

Thanks, now I get why having Map of object references will not work. But then doesn't keeping parent object reference shares the same problem? If it doens't maybe we could keep a map of parent, key tuples.

I'm a little bit picky, as I remember we used to have implementation that was using get trap to identify proxified object, and we discarded it for some reason (I suspect performance)

warpech commented 5 years ago

I made a benchmark for reads, see my last 4 commits.

You are right, this PR introduces about 25% performance degradation in reads. We should care about this, because performance in reads is already terrible compared to lack of proxy (see full results below).

Anyway, I think that a fix for the problem #35 is absolutely necessary. Do you have any other suggestions?

Before this PR

Observe and generate, small object (JSONPatcherProxy)
 Ops/sec: 94422 ±2.81% Ran 5965 times in 0.063 seconds.
Observe and generate (JSONPatcherProxy)
 Ops/sec: 1924 ±2.29% Ran 190 times in 0.099 seconds.
Primitive mutation (JSONPatcherProxy)
 Ops/sec: 480043 ±2.65% Ran 27032 times in 0.056 seconds.
Complex mutation (JSONPatcherProxy)
 Ops/sec: 6996 ±5.30% Ran 421 times in 0.060 seconds.
Serialization (JSONPatcherProxy)
 Ops/sec: 1252 ±0.77% Ran 66 times in 0.053 seconds.

After this PR

Observe and generate, small object (JSONPatcherProxy)
 Ops/sec: 94633 ±2.61% Ran 5925 times in 0.063 seconds.
Observe and generate (JSONPatcherProxy)
 Ops/sec: 2010 ±2.06% Ran 194 times in 0.097 seconds.
Primitive mutation (JSONPatcherProxy)
 Ops/sec: 414442 ±1.92% Ran 22812 times in 0.055 seconds.
Complex mutation (JSONPatcherProxy)
 Ops/sec: 6106 ±4.07% Ran 350 times in 0.057 seconds.
Serialization (JSONPatcherProxy)
 Ops/sec: 935 ±0.82% Ran 49 times in 0.052 seconds.

Full results after this PR

Observe and generate, small object (noop)
 Ops/sec: 12577915 ±0.20% Ran 641041 times in 0.051 seconds.
Observe and generate, small object (JSONPatcherProxy)
 Ops/sec: 92129 ±2.62% Ran 5823 times in 0.063 seconds.
Observe and generate, small object (fast-json-patch)
 Ops/sec: 69657 ±1.80% Ran 3888 times in 0.056 seconds.
Observe and generate (noop)
 Ops/sec: 160466 ±0.26% Ran 8364 times in 0.052 seconds.
Observe and generate (JSONPatcherProxy)
 Ops/sec: 1958 ±1.39% Ran 236 times in 0.121 seconds.
Observe and generate (fast-json-patch)
 Ops/sec: 1698 ±1.10% Ran 93 times in 0.055 seconds.
Primitive mutation (noop)
 Ops/sec: 2549711 ±0.60% Ran 130739 times in 0.051 seconds.
Primitive mutation (JSONPatcherProxy)
 Ops/sec: 403460 ±3.16% Ran 22659 times in 0.056 seconds.
Primitive mutation (fast-json-patch)
 Ops/sec: 7835 ±0.20% Ran 401 times in 0.051 seconds.
Complex mutation (noop)
 Ops/sec: 6551384 ±0.20% Ran 331525 times in 0.051 seconds.
Complex mutation (JSONPatcherProxy)
 Ops/sec: 6035 ±6.15% Ran 354 times in 0.059 seconds.
Complex mutation (fast-json-patch)
 Ops/sec: 7707 ±0.62% Ran 406 times in 0.053 seconds.
Serialization (noop)
 Ops/sec: 7206 ±0.41% Ran 375 times in 0.052 seconds.
Serialization (JSONPatcherProxy)
 Ops/sec: 948 ±0.35% Ran 50 times in 0.053 seconds.
Serialization (fast-json-patch)
 Ops/sec: 7101 ±0.40% Ran 373 times in 0.053 seconds.

tomalec commented 5 years ago

Have you tried

But then doesn't keeping parent object reference shares the same problem? If it doesn't maybe we could keep a map of parent, key tuples.

tomalec commented 5 years ago

I agree it's necessary to fix that problem over the performance, but we should do our best not to degrade it too much. Therefore if the idea of keeping a map of parent+key does not work, let's merge it.

warpech commented 5 years ago

I fixed the generate calls. Current numbers:

After this PR

Observe and generate, small object (JSONPatcherProxy)
 Ops/sec: 95913 ±2.54% Ran 5936 times in 0.062 seconds.
Observe and generate (JSONPatcherProxy)
 Ops/sec: 2008 ±1.49% Ran 210 times in 0.105 seconds.
Primitive mutation (JSONPatcherProxy)
 Ops/sec: 436081 ±0.48% Ran 22251 times in 0.051 seconds.
Complex mutation (JSONPatcherProxy)
 Ops/sec: 7526 ±0.37% Ran 387 times in 0.051 seconds.
Serialization (JSONPatcherProxy)
 Ops/sec: 916 ±0.29% Ran 47 times in 0.051 seconds.

Some numbers acutally went up, because before this commit, the queue of recorded patches was growing infinitely. Now, the queue is flushed after each repetition.

warpech commented 5 years ago

I pushed an additional test to check if double proxification by JSONPatcherProxy works as intended. It does work.

warpech commented 5 years ago

I am not merging this until we find it absolutely necessary for view binding.

One thing that I find very annoyning about this change is how it affects debugging. Any attempt to read a property from the observed object causes the debugger to step into the trapForGet.

warpech commented 5 years ago

I made a new PR that contains only the test refactorings from the current PR: https://github.com/Palindrom/JSONPatcherProxy/pull/42

warpech commented 5 years ago

This PR was rebased on current master and is now available as a new PR #43

Palindrom / JSONPatcherProxy