FlatCache icon indicating copy to clipboard operation
FlatCache copied to clipboard

NSHashTable weak objects releasing

Open toohotz opened this issue 6 years ago • 4 comments

Ahoy there @rnystrom, while sifting through your nice collection of repos, I came across this one which I see is still being worked on and just wanted to comment a bit on this guy below.

https://github.com/GitHawkApp/FlatCache/blob/5700835791eeae29314d215926fd51582bbe3440/FlatCache/FlatCache.swift#L71

I recently came across NSHashTable and MSMapTable and found their ideas to be quite useful then I tested it out and found some issues when using weak references within the tables such as the ones found in the link below and a few other places ☹️

A quick playground of testing out what others spoke about seems to hold true at least in terms of when the object within the table is released

Imgur http://cocoamine.net/blog/2013/12/13/nsmaptable-and-zeroing-weak-references/

Then I got a little curious when I saw your implementation of FlatCache from Soroush and was wondering (if you had freetime of course) to test out the usage of FlatCache and it working as expected? I like the idea that the tables provide the flexibility of using both strong and weak keys and values interchangeably but didn't seem to work as intended all the time.

Cheers to implementing FlatCache, I'll be lurking for its release 😉

toohotz avatar Jun 26 '18 16:06 toohotz

Interesting! Definitely learned something new.

It seems like once you leave the scope of an autoreleasepool, the objects do get deallocated. However if the NSHashTable is stored in a singleton, that means the objects will never be deallocated.

It might be a better idea for me to create my own container object with weak refs and its own compaction in this case...

Here's a playground that shows the behavior difference when you comment/uncomment the pool:

import Foundation

var count = 0

class MyObject: NSObject {
    override init() {
        super.init()
        count += 1
    }
    deinit {
        count -= 1
    }
}

let table = NSHashTable<NSObject>.weakObjects()

//autoreleasepool {
    var o1: NSObject? = MyObject()
    var o2: NSObject? = MyObject()

    count

    table.add(o1)
    table.add(o2)

    table.count
    table.allObjects

    count
//}

count

table.count
table.allObjects

count

table.removeAllObjects()
table.count
table.allObjects

count

rnystrom avatar Jul 09 '18 18:07 rnystrom

@toohotz if you're interested in contributing to this library, that'd be so helpful! Maybe adding some tests that prove the bug, then we can fix it?

rnystrom avatar Jul 09 '18 18:07 rnystrom

Would gladly like to as I have an appreciation to when caching just works 😉

So this post turned out a bit different as I decided to poke around and implement just a working version of removing a value from the storage cache and also stopping listeners from observing a model as well. I'll point to my fork that I had where I tested a few things out to make sure the NSHashTable works as expected which it seems to work fine (using the tests) where the count of the receivedItemQueue quickly showed this.

I assume you had intentions to implement the remove at some point and your coding style differs quite a bit from my own of course but I played around while trying to keep consistent to what you already have implemented so far. It's of course no means a fleshed out solution yet as I have not tested the list which is just a collection of Cachable objects (funny note I just realized now but autocorrects tries to correct Cachable -> Cacheable which I suppose is technically correct?

Anyhow, if you did have plans and not enough time and this is the direction that you're heading in then we can discuss a bit on cleaning the code up more to how you'd expect it. Things as in the throwing function for removing a value I chose to do so because naturally when calling removeValue on a collection, you get back an optional object back if it exists but looking at DecodingError when I switched from SwifyJSON to using the native Decodable by Apple, I found it useful at times to know the offending key that was being used to lookup with. Again, feel free to suggest the direction you feel that this should go in but just wanted to sanity test the NSHashTable bits and it works as intended. On a side note, noticed @khanlou is the guy that I saw at last year's try! Swift in NYC showing off a rather nice rendition of sudoku written in a playground

MyFork

toohotz avatar Jul 11 '18 03:07 toohotz

@toohotz definitely open to considering more broad changes to the framework! Though I've found changing too many things at once makes it infinitely more difficult to critically understand fixes/changes. Let's change one thing at a time.

If you'd like to submit a PR proposing how to fix this, I'd love to take a look!

rnystrom avatar Jul 14 '18 15:07 rnystrom