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

Experiencing lock-up (memory leak, +4MB/s, 30% CPU) when including this after knockout in chrome. #17

Closed
gbmhunter opened this issue Dec 3, 2013 · 17 comments

Comments

@gbmhunter
Copy link

Running chrome with this and the knockout library included in the HTML page, served with XAMPP for Windows.

@gbmhunter
Copy link
Author

O.K., I've narrowed it down to a single instance of the series of JavaScript files that are called using this plugin. Will post more info as I narrow it down further.

@gbmhunter
Copy link
Author

Narrowed down further, it has something to do with binding two comboboxes (<select> object in HTML) to the same ko.observable(). Only freezes when the knockout-deferred-updates library is included.

@mbest
Copy link
Owner

mbest commented Dec 4, 2013

That's interesting. It could be a recursive update issue. Knockout prevents recursive updates to computed observables, and this plugin should also prevent most recursive updates, but it's possible that some can get through.

@mbest
Copy link
Owner

mbest commented Dec 4, 2013

Okay, it's easy to duplicate:

<select data-bind="options: ['abc','def','ghi'], value: x"></select>
<select data-bind="options: ['xyz','uvw'], value: x"></select>

<script>
ko.applyBindings({ x: ko.observable() });
</script>

@mbest
Copy link
Owner

mbest commented Dec 4, 2013

I haven't been able to think of a way to efficiently detect this type of recursion where computed A caused computed B to update, which updates A, and so on. Instead I've added a check that limits the maximum number of task "groups" and throws an exception if it goes over the limit (ad5a8bf). Thus it stops infinite recursion without blocking other types of complex scenarios.

In your particular case, the problem starts with the fact that the value binding tries to make the observable value equal to the currently selected item in the list. Because two different lists are bound to the same observable, this is really impossible, and the observable simply switches endlessly between the two values.

@gbmhunter
Copy link
Author

Ah o.k. So knockout can easily detect this (and prevents it), but when using this plugin this becomes harder to detect?

Also note that when done with plain knockout, it kind of worked as expected, if I selected something from one list, the other list would update automatically (and the ko.observable() got updated).

How would you get two select lists to follow each other without hitting this recursion problem? Sorry if this seems obvious, I am pretty new to Javascript, I come from an embedded background

@mbest
Copy link
Owner

mbest commented Dec 4, 2013

Knockout blocks recursive updates because it was easier to keep track of dependencies that way (knockout/knockout#387). This change was made in 2.1.0, and as you can see here (http://jsfiddle.net/9j2BQ/1/), with 2.0.0, we get an error from the browser (stack size). Even with 3.0.0 (http://jsfiddle.net/9j2BQ/), you can see in the console that the observable value switches between the two values whenever you change it.

I suggest you redesign your code to accurately represent the logic you desire. Otherwise, you're depending on the recursion being stopped at the right point (which could easily change in the future).

@gbmhunter
Copy link
Author

I know this isn't probably the best place to ask, but incase there is a quick answer, how would I link two option (select) lists to the same value correctly i.e. if the user changes one the other one changes, and vice versa?

@mbest
Copy link
Owner

mbest commented Dec 6, 2013

Given the example I provided where you want an observable to equal the last selected item from two select boxes with different available options, I'd use three observables, one each bound to the select boxes, with manual subscriptions from them to update the third.

var vm = {
    x1: ko.observable(),
    x2: ko.observable(),
    x: ko.observable()
};

function updateX(value) {
    vm.x(value);
}
vm.x1.subscribe(updateX);
vm.x2.subscribe(updateX);

http://jsfiddle.net/mbest/9j2BQ/2/

This might not be the best approach for you, though. I'd need to know more about your specific use-case to answer that.

@gbmhunter
Copy link
Author

Hmmm, although it updated the underlying variable, it doesn't update the other select box when one is changed?

I am writing a online calculator framework called candy-calc (https://github.com/gbmhunter/candy-calc). Some of the calculators can be seen here (http://cladlab.com/electronics/general/online-calculators).

As you can see, each variable has selectable units. This is for linking the units of two calculator variables together, so if you change one, the other will change also (both in the backend variable and the front-end display).

By the way, I really appreciate your help here.

@mbest
Copy link
Owner

mbest commented Dec 6, 2013

I see the units select boxes, but I don't see any where the units are linked together.

@gbmhunter
Copy link
Author

That's what I'm trying to do. For the standard resistance calculator, I had the desired and actual resistance units linked together, so if you change the desired resistance units from Ohms to kOhms, the actual resistance units would change also. I did this by just pointing to the same ko.observable(). But when using the deferred updates plugin, this is what caused the crash, and so I'm looking for a proper way to re-implement this. Currently the two unit select boxes are independent.

@mbest
Copy link
Owner

mbest commented Dec 6, 2013

Having both select boxes linked to the same value will work as long as the options are actually the same: http://jsfiddle.net/9j2BQ/3/

You have the options bound to objects, so they need to be the same objects.

@gbmhunter
Copy link
Author

I still need two seperate unit variables though. They need to be linked in the code behind. This is what I have:

    this.linkUnits = function(calcVar1, calcVar2)
    {
        console.log('Linking units...');
        console.log('calcVar1.selUnit = ');
        console.log(calcVar1.selUnit());
        console.log('calcVar2.selUnit = ');
        console.log(calcVar2.selUnit());

        // Modified write function
        calcVar1.dispSelUnit = ko.computed({
            read : function(){
                console.log('Reading source var sel unit.');

                // Return as usual
                return calcVar1.selUnit();
            },
            write : function(value){
                // Write to both of them
                console.log('Writing ' + value + 'to both sel unit.');
                calcVar1.selUnit(value);
                calcVar2.selUnit(value);
            },
            owner : this
        });

        // Modified write function
        calcVar2.dispSelUnit = ko.computed({
            read : function(){
                console.log('Reading destination var sel unit.');

            // Make it equal to the source variables selected unit
                return calcVar2.selUnit();
            },
            write : function(value){
                // Write to both of them
                console.log('Writing ' + value + 'to both sel unit.');

                calcVar1.selUnit(value);
                calcVar2.selUnit(value);
            },
            owner : this
        });
    }

Where .selUnit() are just ko.observable().

But it just forever loops (i.e. recursive issues). I cannot see what is wrong with this!

@mbest
Copy link
Owner

mbest commented Dec 7, 2013

You might be interested in this related question on Stack Overflow: http://stackoverflow.com/q/10809085/1287183

You're getting the recursion problem because the two variables have a different list of units. Although they display the same units, they are actually different objects. You need both variable to reference the same unit objects. In standard-resistance-finder.js, do this:

var resistenceUnits = [
    new cc.unit('m\u2126', 0.001),
    new cc.unit('\u2126', 1.0),
    new cc.unit('k\u2126', 1000.0)
];

this.desiredRes = new cc.input(
    this,
    function() { return true; },
    resistenceUnits,
    0);

...

this.actualRes = new cc.output(
    this,
    ...
    resistenceUnits,
    0, 2);

@gbmhunter
Copy link
Author

@gbmhunter
Copy link
Author

You fixed it! Genius! I'll accept your answer on Stack Overflow. Many thanks!!! Interesting bug...!

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