Adding triple equals check to avoid worst case

[KamranAsif]

Currently we walk through the entire subtree of the object. This change allows us to exit early if we see two objects with the same reference at a higher level.

I wasn't able to get the tests to work locally

[tomalec]

Thanks for feedback, but I'm not quite sure I get your case. Could you add a test for that?

I wasn't able to get the tests to work locally

At which step it fails? https://github.com/Starcounter-Jack/JSON-Patch/blob/master/CONTRIBUTING.md#testing

[KamranAsif]

All tests are failing both in node and in browser, with this error:

OPERATION_PATH_UNRESOLVABLE: Cannot perform the operation at a path that does not exist  OPERATION_PATH_UNRESOLVABLE: Cannot perform the operation at a path that does not exist
      at JsonPatchError.Error (native)
      at new JsonPatchError (JSON-Patch/src/json-patch-duplex.js:493:32)
      at Object.validator (JSON-Patch/src/json-patch-duplex.js:560:27)
      at Object.apply (JSON-Patch/src/json-patch-duplex.js:433:34)
      at Object.validate (JSON-Patch/src/json-patch-duplex.js:587:23)
      at Object.<anonymous> (JSON-Patch/test/spec/validateSpec.js:273:31)
      at attemptSync (JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1950:24)
      at QueueRunner.run (JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1938:9)
      at QueueRunner.execute (JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:1923:10)
      at Spec.queueRunnerFactory (JSON-Patch/node_modules/jasmine-core/lib/jasmine-core/jasmine.js:714:35)

I'm not sure how to add a test for this change, since its a performance improvement instead of a normal bug fix. For example:

var largeObj = {....}
compare(largeObj, largeObj); //should be O(1) instead of O(n)

[tomalec]

I'm not sure how to add a test for this change, since its a performance improvement instead of a normal bug fix. For example:

var largeObj = {....}
compare(largeObj, largeObj); //should be O(1) instead of O(n)

Thanks, that makes a perfect sense. However we could add another benchmark for it

---

All tests are failing both in node and in browser, with this error:

That's wierd, I have just cloned a fresh repo,and everything works fine.

git clone https://github.com/Starcounter-Jack/JSON-Patch.git
cd JSON-Patch
npm install
npm test
npm run bench-duplex

Could you provide a full log?
In the meantime I'd try to write a new bench

[tomalec]

I have added such case 3a020d0 Please tell me, is that what you were talking about?

The difference is huge! So, we will definitely take this. But I'd like to check that there is nothing more to add.

[KamranAsif]

@tomalec Looks like the tests are working. Must have been something about my fork.

The benchmarks look good. Might be worth adding a test for large arrays as well.

Out of curiosity, do you have some before & after numbers?

[KamranAsif]

Would it be worth moving the check to the top of _generate, so we can capture the case of

var largeIntArray = [....]
compare(largeIntArray, largeIntArray); //This will still run in O(n) time with our change

[tomalec]

Would it be worth moving the check to the top of _generate

Would you mind doing that?

[tomalec]

Out of curiosity, do you have some before & after numbers?

Here you have numbers before https://travis-ci.org/Starcounter-Jack/JSON-Patch#L637

You can check numbers after by merging with master :)

[tomalec]

@KamranAsif please let me know once you are finished with changes, so I'll merge this.

[alshakero]

Moving to top made it 123% faster! (37402/30403 op/sec) 🎉

[KamranAsif]

Done :)

[tomalec]

But in total this change for object deep for 11 nodes gave us, waaay much better result
37 402 : 60 083 511
Thanks a lot @KamranAsif

[tomalec]

Released as 1.1.7