Skip to content
This repository was archived by the owner on Jun 19, 2022. It is now read-only.

Back WeakDictionary with NSMapTable#3

Draft
LinusU wants to merge 2 commits intonicholascross:masterfrom
LinusU:NSMapTable
Draft

Back WeakDictionary with NSMapTable#3
LinusU wants to merge 2 commits intonicholascross:masterfrom
LinusU:NSMapTable

Conversation

@LinusU
Copy link
Copy Markdown

@LinusU LinusU commented Jun 1, 2019

I was toying with the idea of backing the WeakDictionary with NSMapTable, and it seems to be working. Unfortunately, I'm running into this problem with NSMapTable, and at this point, I'm starting to wonder if maybe NSMapTable is just broken 🤔

Any ideas? 😄

@LinusU
Copy link
Copy Markdown
Author

LinusU commented Jun 2, 2019

Okay, so this was actually expected behavior, and the values are released as soon as the current autorelease pool is. I reworked and polished my approach, and I think it works quite well now!

It still seems like there is a quirk with how valuesRetainedByKey works together with NSMapTable, need to throw an extra pair of 👀 on that. I thought that just doing valueOptions: valuesRetainedByKey ? .strongMemory : .weakMemory would work, but maybe not 🤔

Most of the tests are currently failing because it expects a) it expects .count to reflect unreaped nil references, and b) it expects values to be released immediately.

I have some local changes that make almost all tests pass, by sprinkling autoreleasepool and changing counts in the tests 😄 It could potentially be worth just rewriting the tests since they mostly seems to focus on the reaping, which isn't there anymore...

I'll push a commit with the semi-passing tests soon.

@nicholascross In general, how do you feel about this approach?

@nicholascross
Copy link
Copy Markdown
Owner

@LinusU Thanks Linus, I will take a look at this PR tonight.

@nicholascross
Copy link
Copy Markdown
Owner

I remember having similar thoughts at some point but TBH it has been a long time since I thought about it. This article from back in the day highlights some of the pitfalls with NSMapTable not sure if it still holds true.

In some respects you could draw similarities between intermittent reaping and draining of the autorelease pool.

Whilst I appreciate you are making an easier to use API by removing the need to reap orphaned references adding the NSMapTable backing storage goes against the unstated goals I had with this library.

  • It should be 100% swift
  • It should ideally not rely on Objective-C foundation

Maybe it is worth considering though, what your goal is.

You want auto reaped references that NSMapTable provides plus potentially some of the difference mentioned here?

  • Use of equality operator for key comparison
  • Supports suscripts and Collection protocol inherited behavior
  • Keys can optionally retain values as long as the key itself is retained the associated value can be retained
  • Manual nil reference clean up required. Weak references are reclaimed as normal but container objects are left holding nil references until reaping is triggered

Would it be possible to extend or wrap NSMapTable with only the feature you are after?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants