Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why do you need the digest call? #9

Closed
bahmutov opened this issue Apr 8, 2016 · 4 comments
Closed

Why do you need the digest call? #9

bahmutov opened this issue Apr 8, 2016 · 4 comments

Comments

@bahmutov
Copy link

bahmutov commented Apr 8, 2016

Hi Curran,

Looking at the examples and wanted to ask - if you know the data dependencies in the properties, that is if "a" or "b" changes, change "a + b" property - why do you need the digest call at all? Can't you recompute the derived properties right away?

model.addPublicProperty('a')
  .addPublicProperty('b')
model({
  'a + b': [(a,b) => a + b, 'a, b']
})
model.a(100) 
// model['a + b'] not computed yet

I think the performance issues might be not too bad

@curran
Copy link
Member

curran commented Apr 9, 2016

Oh wow it's cool to see ES6 syntax with this.

If the derived properties were computed right away - meaning synchronously - then certain cases break. Consider this case from the tests:

  it("Should evaluate tricky case.", function (){

    var model = ReactiveModel()
      .addPublicProperty("a", 1)
      .addPublicProperty("c", 2);

    // a - b
    //       \
    //        e
    //       /
    // c - d

    model({
      b: [increment, "a"],
      d: [increment, "c"],
      e: [add, "b, d"]
    });

    ReactiveModel.digest();

    assert.equal(model.e(), (1 + 1) + (2 + 1));
  });

Imagine that properties a and c both get set like this:

model.a(10);
model.c(20);

If the changes were propagated through right away (synchronously after each change), then e would be set to an invalid intermediate state after a is set and before c is set.

This is exactly the fundamental flaw with model.js that I wanted to solve with this library. This issue would come up in visualizations and would result in inconsistent intermediate states. For example, a change in width would propagate all the way through to the SVG DOM before a change in height. Since this library uses topological sort, and only executes a digest on the next tick or when you invoke digest(), any set of changes that happen synchronously (e.g. vis.width(300).height(500)) will propagate through correctly with no invalid intermediate states.

Here's another test case that would fail if changes were propagated synchronously, from the reactive-function tests.

  it("Should handle tricky case.", function () {

    // This is the case where model-js failed (https://github.com/curran/model),
    // which cropped up as flashes of inconstistent states in Chiasm visualizations.
    // For example, it happens when you change the X column in this example
    // "Magic Heat Map" http://bl.ocks.org/curran/a54fc3a6578efcdc19f4
    // This flaw in model-js is the main inspiration for making this library and using topological sort.
    //
    //      a
    //     / \
    //    b   |
    //    |   d
    //    c   |
    //     \ /
    //      e   
    //
    var a = ReactiveProperty(5);

    var b = ReactiveProperty();
    var c = ReactiveProperty();
    var d = ReactiveProperty();
    var e = ReactiveProperty();

    ReactiveFunction({ inputs: [a],    output: b, callback: function (a){    return a * 2; } });
    ReactiveFunction({ inputs: [b],    output: c, callback: function (b){    return b + 5; } });
    ReactiveFunction({ inputs: [a],    output: d, callback: function (a){    return a * 3; } });
    ReactiveFunction({ inputs: [c, d], output: e, callback: function (c, d){ return c + d; } });

    ReactiveFunction.digest();
    assert.equal(e(), ((a() * 2) + 5) + (a() * 3));

    a(10);
    ReactiveFunction.digest();
    assert.equal(e(), ((a() * 2) + 5) + (a() * 3));
  });

@curran curran closed this as completed Apr 9, 2016
@bahmutov
Copy link
Author

Hmm, what about introducing "compound" properties that should be set at the
same time to solve this problem?

model.addCompounProperty('w', 'h')
// then must use "compound" wh property to set both at once
model.wh(200, 300)

As far as updating the DOM, I think the preferred solution would be to NOT
do immediate DOM update and instead use requestAnimationFrame - this would
postpone the DOM writing and give a chance for several properties to
update?

On Sat, Apr 9, 2016 at 4:14 AM, Curran Kelleher notifications@github.com
wrote:

Oh wow it's cool to see ES6 syntax with this.

If the derived properties were computed right away - meaning synchronously

  • then certain cases break. Consider this case from the tests:

    it("Should evaluate tricky case.", function (){

    var model = ReactiveModel()
    .addPublicProperty("a", 1)
    .addPublicProperty("c", 2);

    // a - b
    //
    // e
    // /
    // c - d

    model({
    b: [increment, "a"],
    d: [increment, "c"],
    e: [add, "b, d"]
    });

    ReactiveModel.digest();

    assert.equal(model.e(), (1 + 1) + (2 + 1));
    });

Imagine that properties a and c both get set like this:

model.a(10);model.c(20);

If the changes were propagated through right away (synchronously after
each change), then e would be set to an invalid intermediate state after a
is set and before c is set.

This is exactly the fundamental flaw with model.js that I wanted to solve
with this library. This issue would come up in visualizations and would
result in inconsistent intermediate states. For example, a change in width
would propagate all the way through to the SVG DOM before a change in
height. Since this library uses topological sort, and only executes a
digest on the next tick or when you invoke digest(), any set of changes
that happen synchronously (e.g. vis.width(300).height(500)) will
propagate through correctly with no invalid intermediate states.

Here's another test case that would fail if changes were propagated
synchronously, from the reactive-function tests
https://github.com/curran/reactive-function/blob/master/test.js#L124.

it("Should handle tricky case.", function () {

// This is the case where model-js failed (https://github.com/curran/model),
// which cropped up as flashes of inconstistent states in Chiasm visualizations.
// For example, it happens when you change the X column in this example
// "Magic Heat Map" http://bl.ocks.org/curran/a54fc3a6578efcdc19f4
// This flaw in model-js is the main inspiration for making this library and using topological sort.
//
//      a
//     / \
//    b   |
//    |   d
//    c   |
//     \ /
//      e
//
var a = ReactiveProperty(5);

var b = ReactiveProperty();
var c = ReactiveProperty();
var d = ReactiveProperty();
var e = ReactiveProperty();

ReactiveFunction({ inputs: [a],    output: b, callback: function (a){    return a * 2; } });
ReactiveFunction({ inputs: [b],    output: c, callback: function (b){    return b + 5; } });
ReactiveFunction({ inputs: [a],    output: d, callback: function (a){    return a * 3; } });
ReactiveFunction({ inputs: [c, d], output: e, callback: function (c, d){ return c + d; } });

ReactiveFunction.digest();
assert.equal(e(), ((a() * 2) + 5) + (a() * 3));

a(10);
ReactiveFunction.digest();
assert.equal(e(), ((a() * 2) + 5) + (a() * 3));

});


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#9 (comment)

Dr. Gleb Bahmutov, PhD

gleb.bahmutov@gmail.com @bahmutov https://twitter.com/@bahmutov
https://glebbahmutov.com/ https://glebbahmutov.com/blog
https://github.com/bahmutov

@curran
Copy link
Member

curran commented Apr 11, 2016

This library is for exactly this kind of case, where there are functions that depend on multiple properties that can be thought of as compound properties, that change together. The case of width and height works well like this:

var my = ReactiveModel()
  .addPublicProperty("width", 50)
  .addPublicProperty("height", 60);

my({
  dom: [function (width, height){
    ... use D3 or React to update some DOM that depends on width and height.
  }, "width, height"]
});

my.width(500);
my.height(600);

The function invocation is deferred until the next tick, so setting width and height together like this will cause the function to be invoked with both of the new values.

I considered using requestAnimationFrame to implement the digest debounce. Currently the library uses setTimeout to debounce to the next tick. I think in most cases the performance of these two would be roughly the same. It would be interesting to do some tests though, for example with mouse movement, or a D3 brush - how many mouse events come in between each tick vs between each animation frame. I recall the delay with setTimeout(fn, 0) is roughly 4 ms, while the delay between animation frames is about 16 ms. In theory this implies that a DOM updating function like the one above could conceivably execute 3 or 4 times between successive animation frames, and all but the last one would be unnecessary (assuming user's display updates at 60 FPS).

@curran
Copy link
Member

curran commented Apr 14, 2016

Thank you @bahmutov for the requestAnimationFrame suggestion. This has been implemented in the latest release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants