kb.Store::retainOrCreate() calls kb.Store::retain() twice when 'creating' and never when 'retaining'

[piusj]

Hi kmalalkoff,

I'm a big fan of this library. There's just one thing which is breaking our very complex app.

The quick version of this issue, is that retain() is called twice when a new viewModel is registered with retainOrCreate (once at the end of this function, line 112; and once when the viewModel is created, via useOptionsOrCreate(), line 33). When a viewModel already exists, i expect retain to be called, but instead the function just returns the found observable, line 103.

• The lines above refer to src/core/store.coffee.

I'm not sure if this is how you intended to use the retain function, but let me tell why i expect this behaviour.

My complex App uses a global kb.Store to share all my viewModels. I am using Backbone.Relational to handle my Model Hierarchy.
Many of my models have a complex hierarchy of nested models (no circular structures) and sometimes I want to set a nested model, to another model. When I set a nested model to another one, the previous value gets released. Quite often that previous value is a shared viewModel which appears in another collectionObservable or nested in another model. Each time a shared ViewModel is released, the refCount is reduced until it is eventually destroyed, breaking the app.

Think of this simple scenario. Job is a viewModel, Client is a viewModel, Job has a Client as a nested model. User changes JobA.client() from ClientA to ClientB. ClientA gets destroyed and any other Jobs that held that client now references a __kb_released ViewModel. (* More accurately, you need to change the client back and forth twice because the ref_count starts at 2.)

Theres an easy way to fix this problem. 1) add a ref_count using retain whenever a viewModel is created or set on a parent viewModel (or simply call retain() when an observable is found in retainOrCreate). The double ref_count on creation is a less significant problem, but seems like a bug.

I've been logging the ref_count changes in some of my tests and i expect the changes to look like 1, 2, 3, 4, 3, 2, 1, destroyed. But they actually look like 2, 2, 2, 2, 1, destroyed.

Note that this only happens when I specify a custom global kb.Store on all my viewModels. If I don't do this and let the Framework create the stores for me, the viewModels don't get shared and way too many viewModels get created, freezing up the browser significantly.

I know the change that needs to be made to retainOrCreate (call retain when existing observable found, but not otherwise). Let me know if you agree and would like me to fork and send a pull request.

This issue is a bit of a show stopper for us, but everything else about this framework is like poetry. Great work!

[kmalakoff]

I'm glad to hear that Knockback is a pleasure to use despite this issue. Also, I really appreciate the time you spent to understand and explain the problems. Thank you!

The store was implemented for handling circular dependencies in distinct pools of view models for dependent sub-sections of an application. Also, the design in mind was that there was a one-to-many relationship between models and view models since different parts of the app would have different hierarchical display needs rather than having a one-to-one mapping between models and view models causing view models to be Uber view models for all situations.

Because of these design goals, I wasn't anticipating anyone using one global store for the full app. Also, the creation cycle of first going deep through the hierarchy before getting and retaining references to view models made reference counting a little non-conventional in the order of operations around lifecycles. This is the likely reason for the order of operations that you mentioned.

Also, because view models are shared, the 1-2-3-4-3-2-1 vs 1-2-2-2-2-1 patterns seems like it might be expected if a globally shared view model is retaining one count to a dependent view model. Of course, if this isn't the case, then the handling may be wrong.

Hopefully, this information provides enough detail to make a decision on changing the reference counting logic to try to make a global store behave as you expect be spite these potential gotchas. I'm definitely not opposed to a fresh look at the reference counting code, would accept a pull request if the tests pass, and believe the tests will most likely catch potential breaking behavior cases.

Other options you could consider:

• no global store and one-to-many relationships between models and view models
• manually retaining view models before a batch of changes and then releasing afterwards. Although this sounds like it may still suffer from the problem due to nested references being over-zealously released.

Let me know what you decide and if you want to discuss any details you discover.

[piusj]

The no global store and one-to-many relationship between models and view models option, was actually what we had, which was working great until we hit performance issues. It makes a lot of sense to have it that way, but unfortunately for our app, the many situations caused too many view models to be created.

I added the global store to reuse the view models because our design turns out to have 'Uber view models' anyway. e.g.

class Job extends extends kb.ViewModel
   constructor: (model) ->
      super(model, {factories: Client, store: GlobalKbStore})

   getDisplayName: -> @name() + @client().getDisplayName()

class Client extends kb.ViewModel
   constructor: (model) ->
      super(model, {store: GlobalKbStore})

   getDisplayName: -> @name()

class JobList extends kb.CollectionObservable
   constructor: (collection) ->
      super(collection, {view_model: Job, store: GlobalKbStore})

That's a very simplified example, but when designed this way, it works for both one-to-one and one-to-many type model to view model structures. I handle the context specific behaviour with the controllers for that context.

I would have liked to keep the one-to-many structure with many pools of stores, but the way i've designed it means that too many distinct pools are created and hence almost every reuse of a model creates a new view model (in the thousands in total).

I think the change I wish to make will still work for both types of relationships and I think there is indeed a bug where the first view model creates a double ref_count, where both retainOrCreate and useOptionsOrCreate are calling retain in the same flow. It might be worth checking some of your examples to see if your view models start with a ref_count of 2. If not, it must be something with my design only.

I have personally found that the ref_count pattern 1-2-3-4-3-2-1 never happens in any way its been designed. Its always some variation of 2-2-2-2-1.

I will create a pull request and run the tests for your review.

[piusj]

Hi Kevin,

So i think i've worked it out.

retainOrCreate() gets called from exactly 2 places in knockback.core:

• collection-observable.coffee:417
• typed-value.coffee:81

I think these two calls need to behave differently.

• in collection-observable.coffee, the retainOrCreate() function needs to behave like a findOrCreate. When a matching observable has been found in the store, it should just return the observable.
• In typed-value, the retainOrCreate() function is called when setting an observable with a creator. In this case i think it makes sense to increase the ref_count for this typed value, then release the previous_value. Currently, knockback only releases the previous_value. In this scenario retainOrCreate should behave exactly in the way the name implies - retain if found, create if otherwise.

Since i've applied the fix, in my personal tests the ref_counts seem to increase and decrease when necessary and view_models are released when there are no more references. View models have not been over-zealously released so far.

There is a possibility that some view models may get a higher ref_counts than they should meaning they might never get released. I haven't been able to produce that scenario, and I don't understand the framework well enough to be 100% sure. But if it does come up, i figure its less destructive than releasing when its not supposed to. So you might have to check this if you think it might be an issue. More likely though, I think I might have solved the problem of over-zealously releasing view models entirely (fingers crossed).

My proposed fix is to add an additional parameter to the function - retainOrCreate(obj, options, retainExisting). The boolean value of retainExisting decides whether to retain the found observable or just return it.

I thought about separating this behaviour into two functions - retainOrCreate and findOrCreate, but i figured there would be too much duplicated code and it may break compatibility with other existing code/libraries that may be using the kb.Store.

pull request is here #156

[piusj]

P.S. My app is now blazingly fast in comparison :)

[kmalakoff]

Awesome! Your explanation sounds right.

I'll review the changes and will try to get the tests to pass before pushing out a new release. I'll try to get this done tonight or Saturday morning (PST).

[kmalakoff]

There was one change that I needed to make to get the tests to pass: https://github.com/kmalakoff/knockback/blob/update-refcounting/src/core/store.coffee#L113

Let me know if this makes sense and works for you.

FYI: I've moved to the update-refcounting branch.

[piusj]

Hi Kevin,

I think that may cause the ref_count to start at 2 in some cases, but that's fine with me. If your tests show that there are view models being released, i think its working great.

I'll pull the changes and run some tests on my app.

[piusj]

I have noticed a lot less view models being released. But, I'm really okay with it because I'm reusing them. However i'm not sure if there are others who may be losing the memory management benefit, of releasing unused view models. Once again, if your tests seem to be releasing them fine and as long as your happy with that side of things, I guess we're all good to go :)

[kmalakoff]

Thank you for confirming and explaining about the starting ref count.

It could be that the store itself holds 1 reference and then the collection holding it stores another causing it to start at 2. I'll take another pass through the failing tests without the extra retain and confirm this is the case before releasing a new version of the library, but like you say, if the tests are passing, it should be behaving properly (hopefully!).

[piusj]

You're right! I totally missed that. I'm getting ref_count 1's now

[kmalakoff]

It's released under 1.1.0.

Thank you for tracking this problem down and submitting a fix!

[piusj]

Thank you! You rock!

[kmalakoff]

It looks like this fix breaks #157

We'll triage there if you have time to help...