New subscription system for reference cursors

[tomasd]

Reference cursors are now notified about the change also in case when something on the path to the reference is changed.

[mikaelbr]

Thanks! I'll look more into this and test it out locally today, and merge if everything looks good. I should also probably do a performance test.

@andrewluetgers how about your test here?

[torgeir]

This ought to allow for something like https://github.com/omniscientjs/immstruct/issues/19, no?

[andrewluetgers]

@tomasd does this resolve #37 ?

[tomasd]

@andrewluetgers Yes, it should be. https://github.com/tomasd/immstruct/blob/master/tests/structure_test.js#L859

[dashed]

@andrewluetgers @tomasd No. It doesn't look like it.

To resolve https://github.com/omniscientjs/immstruct/issues/37, something like this would need to be in the test:

        var ref2 = structure.reference(['a', 'b', 'c']);
        ref2.observe(function () { done(); });

[andrewluetgers]

Looks like invoke is dead code now

[andrewluetgers]

Sorry to say it but there is a significant performance regression. This test also does not deal with deeply nested structures so it is not well suited to really put this change to the test, I would assume perf would be even worse with deeply nested structures? At any rate I will need to revamp this test to do just that so we can know for sure.

with current master ~170 fps http://plnkr.co/edit/upGq3HAqeDW6RHgo96IZ?p=preview

with latest changes ~5 fps http://plnkr.co/edit/RdGpH5v6XOnYnvID6w9w?p=preview

[andrewluetgers]

One suggestion, If there is not a fundamental need for using an immutable structure anywhere in your changes, best to opt for native mutable js alternatives in those places.

[dashed]

The performance regression isn't surprising since laziness/shortcut fusion isn't being utilized.

[andrewluetgers]

@Dashed are you implying laziness was being used before or could be? I'm not sure there is much of an opportunity for it here but I'm no expert on that.

Took a look at how it scales as you change the size of the matrix. 10x10 @ 140 fps, 20x20 @ 88 fps, 30x30 @ 44 fps, 40x40 @ 30 fps

[mikaelbr]

I suspected as much. As this iterate much more, it might in cases where there are many listeners get slow. Where we need to improve the performance is at on swap and inside pathCombinations. Those operations are the ones who get called a lot.

[mikaelbr]

If we were to fix the performance issue and extend it to solve #37, it'd be a pretty good day! Will make immstruct a lot more powerful and versatile.

[tomasd]

@andrewluetgers Thank you for the performance report. I'll try to fix it soon. Regarding #37 I've tried example from TLDR and it worked:

describe('xxx', function() {
     it('xxx', function(done) {
         var structure = Structure({
             data: {  }
         });

// this doesn't work as intended. very unexpected when user doesn't know how
// Cursor.from(...) works.
         structure
             .reference(['config', 'profile'])
             .observe(function(newObj, oldObj, hotKeyPath) {
                 // no output
                 done();
             });

// update some object
         structure.cursor().set('config', Immutable.fromJS({
             'profile': 'throwaway',
             'color': 'green'
         }));
     })  ;
})  ;

Or is there any other problem which I've missed? Thanks

[dashed]

Ah. I've misread the code for the subscribe function. Yes, you're right, it resolves https://github.com/omniscientjs/immstruct/issues/37.

---

However, after looking at the code more closely, there are some things that doesn't seem right.

It seems you're populating this._referencelisteners in a lookup table manner:

{ 'List []': [ [Function: cursorRefresher], [Function: listener] ],
  'List [ "config" ]': [ [Function: cursorRefresher], [Function: listener] ],
  'List [ "config", "profile" ]': [ [Function: cursorRefresher], [Function: listener] ] }

Is there a reason for this? It's probably better to use Immutable.Map as a tree to optimize the constant factor in the lookup operation; and use sentinel values (e.g. LISTENERS = {}) to retrieve the listener set.

Looking at https://github.com/omniscientjs/immstruct/pull/60/files#diff-dc0aa2a7df4698297c4ccd9dc0b071caR74, merging Immutable.Set of listeners at every subset of keyPath doesn't seem like it would scale for long keyPaths imo.

---

However, when investigating https://github.com/omniscientjs/immstruct/issues/37 I've thought up a similar solution.

But, I've come to the conclusion that it's best to actually separate the listeners into their respective contexts. To elaborate, at keyPath, there would be two types of listeners:

1. listeners observing keyPath, and
2. listeners observing at path where keyPath is a subpath; that is listeners subscribed to the sub-tree of keyPath.

Then, on swap, you traverse keyPath to notify listeners in (1) in this._referencelisteners. When you finally reached the end, just notify listeners in (2) at keyPath.

There are ways to optimize the second step to avoid traversing all the listeners in (2).

[tomasd]

I've pushed optimized version. It gives about 160 fps on my machine (original master was 170fps).

Listeners are now subscribed into path tree index and called when their path has changed.

[andrewluetgers]

Also BRAVO on the refactor :-)

[tomasd]

I've updated the code according to last comments and squashed the commits. Do you need something more for merge?

[mikaelbr]

Great work! I'd still prefer to have the dist to the current released version, not the unstable version. Could you undo the dist?

[tomasd]

Yes, I've just reverted the dist. Hope it's ok now :-)

[mikaelbr]

LGTM. There are some code stylistic changes I'd like to do, but I can refactor the code. @andrewluetgers what do you think? Did you performance test the new changes?

@Dashed Are you happy with the PR? So #37 and #57 will be fixed for this. Are there any tests that we need to add?

[andrewluetgers]

@mikaelbr looks good there is no difference in performance now http://plnkr.co/edit/FMOIeJ7edrDaTimJ9tst?p=preview

[mikaelbr]

Perfect. I did some changes, refactoring and fixed some issues and added more tests. Merged now! Thanks for all your great work, @tomasd , @andrewluetgers and @Dashed! :cake:

This is a great feature!