kb.Store::retainOrCreate() calls kb.Store::retain() twice when 'creating' and never when 'retaining' about knockback HOT 13 CLOSED

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.

from knockback.

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.

from knockback.

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

from knockback.

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

from knockback.

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).

from knockback.

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.

from knockback.

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.

from knockback.

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 :)

from knockback.

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!).

from knockback.

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

from knockback.

It's released under 1.1.0.

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

from knockback.

Thank you! You rock!

from knockback.

It looks like this fix breaks #157

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

from knockback.