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

NSHashTable weak objects releasing #3

Open
toohotz opened this issue Jun 26, 2018 · 4 comments
Open

NSHashTable weak objects releasing #3

toohotz opened this issue Jun 26, 2018 · 4 comments

Comments

@toohotz
Copy link

toohotz commented Jun 26, 2018

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.

table = NSHashTable.weakObjects()

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 😉

@rnystrom
Copy link
Member

rnystrom commented Jul 9, 2018

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
Copy link
Member

rnystrom commented Jul 9, 2018

@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?

@toohotz
Copy link
Author

toohotz commented Jul 11, 2018

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

@rnystrom
Copy link
Member

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants