Going async or implementing batch patch collection

[alshakero]

Thanks to jsonpatch all our apps assume that tree changes come asynchronously. So going synchronous is in fact the breaking change. We can leverage this and make JSONPatcherProxy asynchronous. Mind the fact that this will not create patches order problems! Because the changes are recorded synchronously and no change can go unnoticed. The asynchronous part only covers calling the callback (sending to the socket), the recording part remains synchronous.

Pros:

1. Big sync changes (eg: loops) will not kill performance by triggering WS.send a lot. They will be batched into one patch.
2. We can very easily track move operation with O(1) performance! Given that we have a map that contains all the objects in our tree that we can look up with O(1) speed, we can try move operation very easily.
3. Summation of patches comes naturally.
4. We will be able to squash redundant changes. Now (obj.size = 1; obj.size = 2) would create two operations in the same patch, one of them is redundant. But we can compress them using my lib (I wrote it this weekend in my free time, I don't mind moving to Palindrom organisation if you agree to invest time in it).
5. No breaking changes in all our apps.
6. We can make it an optional flag with no hassle whatsoever.

Cons:

1. Will take some time to re-write the tests to deal with the async manner.
2. Could be less semantic. Synchronous code is always more predictable.

Implementation:

instead of calling the callback with every change, we can:

clearTimeout(lastTimeout);
patch.push({operation that happened});
lastTimeout = setTimeout(emitPatch, 1, patch);

Yup, that simple.

/cc @warpech @tomalec

[tomalec]

AFAIR, this asynchronous manner was a problem for us many times. However it's a valid point we may have used to it so much, that now sync would become a problem.

I'd either

1. give sync a try, and expose a utility for batching, or
2. implement async as a cheep sugar layer, that could be easily switched off in future. "cheep" would require some benchmark tests

But we can discuss it on the upcoming meeting

[warpech]

Thanks for the idea @alshakero and thanks @tomalec for the counter-idea :)

I think that sync is worth trying in something that we can call Palindrom 3.0.0-rc.1.

If we discover problems with sync, then I strongly propose to re-introduce async on the Palindrom level, not on the JSONPatcherProxy level.

To remix the great simple code from @alshakero, Palindrom would do sth like:

clearTimeout(lastTimeout);
operations.push({operation that came from JSONPatcherProxy});
lastTimeout = setTimeout(sendOperations, 1, operations);

[warpech]

What's the status of this? Is JSONPatcherProxy still sync? Any problems with it so far?

[eriksunsol]

Coming a bit late into this (we switched from PuppetJs to Palindrom just this week due to issues we had with migrating our code, not related to Palindrom though).

Although quite elaborative, this post is about a potential draw-back with sync behavior.

I found this thread because I noticed that patches are now sent over the network immediately when a change happens while patches used to be (with PuppetJs) collected in an event handler capturing mouse and keyboard events and thus multiple patches would be sent in a single request.

I get that this has two main benefits:

1. Patch generation on client side is cheaper now
2. Patches are guaranteed to be generated in the order changes are made

The drawbacks I could see immediately however:

1. Don't change data client-side unless you really mean it, e.g. don't increase a value in a loop, because every increment will generate a patch (already mentioned in the original comment)
2. Every time a patch is sent over the network, the server needs to respond and in doing so it has to run a view model update to collect changes from server side. E.g. Starcounter XSON has no other way of doing that except traversing the entire view model. This can be expensive (in our case it is).

The second drawback is the main reason for posting this comment.

I think an optional async behavior would be beneficial either by leaving it up to the client side application to trigger sending the patch queue, or send it in an event handler like PuppetJs did. The latter made total sense in PuppetJs, because one user interaction would result in a single round-trip to the server. If Palindrom can't assume there is a terminal connected (for e.g. NodeJs?), then leave it up to the application.

I don't like the setTimeout() based solution. Too non-deterministic.

[warpech]

Imagine an API: palindrom.startBatch()/palindrom.endBatch(). When you call startBatch, local changes are detected as you make them but they are queued. They are sent to the server when you call endBatch. Calling startBatch while already in startBatch throws an error.

Would this solve the problem without going async?

[eriksunsol]

@warpech : Yes! Perfect!

As a side note, i just tried a workaround by updating a whole object in my view model, which will result in a single replace patch containing all the properties in the object. Works well from a Palindrom perspective, but Starcounter XSON chokes on it. It can't handle replace on an object property...

I have a complex update scenario where this is much needed.

The only other workaround I can come up with is to encode this complex scenario in a single string property instead of several. That'll work , but the view model will look more obscure.

[alshakero]

I tried going async in a branch. And didn't like it. I had to introduce a generate function that synchronously triggers when needed and so on. As Eric said, it's too non-deterministic.

I agree with the transactional approach. It also gives us room for patch de-redundancy (for the lack of words).

[eriksunsol]

The only other workaround I can come up with is to encode this complex scenario in a single string property instead of several. That'll work , but the view model will look more obscure.

This is what I'll do for now. It has some other benefits too, and may be better for this use case after all.

[alshakero]

@warpech should I go for the palindrom.startBatch()/palindrom.endBatch() idea?

[warpech]

Let's wait for @tomalec's opinion.

@tomalec is this good enough as a replacement for Palindrom/Palindrom#29?

Keep in mind that it is labeled "Nice to have", we don't have to it immediately.