kb.release is too destructive

[idanz]

Using kb.release seems to be a necessity for preventing memory leakage when creating view models.
However it seems too destructive to me. Since I am using custom ViewModel classes that extend kb.ViewModel I encounter many situations where I am loosing shared objects when releasing a viewmodel.
For example kb.release([1,2]), actually empties the array. if the array object is shared between viewmodels, this causes problems.
I would suggest instead of black listing the Backbone Models from the destruction list, why not white list what really needs to be destroyed or disconnected.

[kmalakoff]

Understood. Knockback flags ViewModels so they are deleted twice, but if you are sharing something between two ViewModels that have different life cycles, what you describe will happen. I haven't encountered this situation myself, but white or black listing will help assuming there are no other work arounds on the client side.

If this is really a need, I'd rather blacklist than white list as people customize ViewModels, they shouldn't have to list all observables they add.

Can you make a case for white listing over black listing (pseudo code to describe the benefit)? Either way, what sort of API would you like?

Cheers...

[idanz]

Hi,
Thanks for the quick response, I really appreciate and the effort you put into this project.
I agree, the problem is that people do customize the ViewModels so they might contain objects that are not models and still needs to be shared.
In my case, I actually had a model that contains a complex JS structure and I kept a reference to an inner object in that structure. Since other viewmodels also used this model, they were affected by the deletion of the inner object.

I don't completely understand what needs to be disposed inorder to avoid memory leakage but I assume it's related to event binding on the models that are observerd.

So my proposal is:

1. Whenever you create a "dangerous" link or binding, mark the object that does it (is it not just your kb.Observable and kb.ObservableCollection?)
2. On "release", walk the objects like you do today but only act on those marked object and release them from all the bindings.
3. Now the viewmodels can be disposed properly by the GC

[kmalakoff]

Hi Idan,

Thank you for working through this with me!

Deleting your complex JS structure should be avoidable, but we need to
strike the right balance in use cases.

IZ> I don't completely understand what needs to be disposed inorder to
avoid memory leakage but I assume it's related to event binding on the
models that are observerd.

KM> What needs to be disposed are the Knockout Observables. A ko.computed
or kb.collectionObservable or ViewModel can hold cycles that need to be
manually broken to release memory and also to clean up the Backbone.Events registrations.

The tricky part comes in where you have a hierarchy of ViewModels where one
ViewModel aggregates others so that when the parent ViewModel is released,
the child ViewModels need to also be released, and because the child
ViewModels can be in any form (a kb.ViewModel, a JS vanilla object, or a a
JS new-ed object) and it is not possible to determine whether they are a
ViewModel or not, I just traverse the full hierarchy to release all of the
Knockout Observables (including clearing arrays). I believe we need to
preserve this functionality somehow because it is quite common.

So the combination between the two are causing your problem since Knockback
assumes your object is a potentially-hierarchical ViewModel that needs to
be released.

I think your proposal is a white list proposal as you originally suggested
(I'm assuming "dangerous" == "nested ViewModel"?), but my concern is that
it is the exception and not the general use case given that nested
ViewModels are quite common (I personally use them very often).

So we could reverse your proposed algorithm to be "mark as not a ViewModel"

• this could be done by adding a hash or array on the owning ViewModel so
  as to not edit the nested object/property. Alternatively, I could try to
  cover some of the cases by introducing a tagging mechanism on the nested
  ViewModel itself (or both tagging on the owning ViewModel or nested
  ViewModel) so that if a ViewModel is derived from kb.ViewModel, it is
  automatically known or if it is created through the kb.Store or through
  kb-inject, it is automatically tagged, etc. I'm just worried about adding
  too much bookkeeping boiler plate/complexity into the library, API and use
  cases.

The other option could be to try to not clear arrays unless something in
them was released meaning it was a view model (I'm not sure if this was the
specific problem from your original post or just one of many problems).

Given this latest explanation, does anything stick out to you as the best
tradeoff? I'm sort of leaning towards blacklisting under the assumption
that is it a rare case.

Cheers,

Kevin

On Mon, Oct 29, 2012 at 1:35 AM, Idan Zalzberg notifications@github.comwrote:

Hi,
Thanks for the quick response, I really appreciate and the effort you put
into this project.
I agree, the problem is that people do customize the ViewModels so they
might contain objects that are not models and still needs to be shared.
In my case, I actually had a model that contains a complex JS structure
and I kept a reference to an inner object in that structure. Since other
viewmodels also used this model, they were affected by the deletion of the
inner object.

I don't completely understand what needs to be disposed inorder to avoid
memory leakage but I assume it's related to event binding on the models
that are observerd.

So my proposal is:

1. Whenever you create a "dangerous" link or binding, mark the object that
   does it (is it not just your kb.Observable and kb.ObservableCollection?)
2. On "release", walk the objects like you do today but only act on those
   marked object and release them from all the bindings.
3. Now the viewmodels can be disposed properly by the GC

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/48#issuecomment-9859160.

[idanz]

Thanks for the reply,
I tend to disagree on what is more troubling.
I don't like memory leaks but I dread having my data destroyed unintentionally, just because I added a reference to it. This might be later pushed to the server and cause irreversible changes.

Still, I think I am missing something here, why should the array be erased? I understand they need to traversed and every observer should be cleared from subscriptions and clear itself from the models, But how does clearing the actual arrays help here?

[kmalakoff]

You raise good points - I'm very happy to finally be challenged on these decisions...

When I was writing that code, I just took a strict and exhaustive view of clearing the ViewModels to ensure any potential cycles were broken, but it sounds like I probably went too far assuming ViewModels mainly aggregated observables and other ViewModels!

Let's say that I just cleared the subscriptions and unbound any Backbone events, and still traversed the full object hierarchy, but did not remove elements from arrays and did not clear properties. Would that handle most cycle cases for the JS garbage collector? Or would I still clear properties on objects and arrays elements if they were observables or ViewModels.

Would either of these options solve your problem or just push it to a different place?

Any what about the white listing and black listing? Do you agree that there is stronger case for black listing and would it still be necessary?

I'm happy to implement the best and most expected solution, I just need to know what it is.

[idanz]

I am not an expert on the JS GC but I think it works using a root tree.
So it makes sense that disconnecting from the persistent models (by clearing the relevant events) should make everything collectable.

Since knockback binds to the models on creation, I expect it to unbind when I release, with the same magical recursion I get when I bind.
So, yes, assuming disconnecting from the models is enough to break the cycles it would be very useful to me

[kmalakoff]

Understood. I'll take these discussions, will prepare a less destructive version of release in the next couple of days and when you can verify that it works for you, I'll release it.

Thanks for all your help on this!

[idanz]

Thank you.
For full disclosure I should mention that in the mean time i created my own version of a backbone-knockout adapter.
It's heavily inspired by knockback but the code is not related, It's alot smaller than knockback since it has a lot less features, which makes it easier for me to grasp.
If you are interested I can send my release method which seems to work fine without being destructive.
It's not on GitHub (yet) and I don't know if it will be since I might not have time for the documentation, etc...

[kmalakoff]

Yeah no worries. I'll still do this, but if it isn't as urgent then that's totally better for me!

[kmalakoff]

Reduced destructiveness of kb.release in 0.17.1