Critical: Clean Object Dictionary look over

[pjrobertson]

I've just stumbled across this today, and it's a biggy we shouldn't have overlooked when we merged the ARC stuff in. I seem to remember @craigotis mentioning it, so it's my bad for not raising it...

but the important code in +[QSObject cleanObjectDictionary] is commented out.
This means objects aren't ever going to get released (theoretically)

You can see the memory increase (well, the object increase) if you:

• Pause QS running in Xcode and type print [(NSArray *)[objectDictionary allKeys] count] in the debugger
• Resume Xcode, and browse to /usr/bin/ in QS (and/or any other place that makes lots of 'temporary' QSObjects)
• Maybe wait a little while for cleanObjectDictionary to be called/any cleanup to be performed (it's called when the interface closes, no need to wait if you trust me :D)
• Pause Xcode again and run print [(NSArray *)[objectDictionary allKeys] count]
• You'll see the number of objects being stored has gone up quite a bit.

cleanObjectDictionary was never the right way to go about this. But what it? :)

[craigotis]

I'm familiarizing myself with the QSObject class. Obviously, the commented-out code won't work - as you mention, checking the retain count probably wasn't the right way in the first place. But, what is? :)

As best I can tell, the only call that actually stores items in the objectDictionary in the QSObject class (excluding the setIdentifier: method, which just performs a quick key replacement) is:

+ (void)registerObject:(QSBasicObject *)object withIdentifier:(NSString *)anIdentifier;

Which is only ever used to register self with some identifier. Thus, could we not just add a line to the dealloc method in QSObject to unregister itself?

Should it not be the responsibility of the objects to unregister themselves, rather than checking retain counts and pulling them out?

[craigotis]

Something like this:

craigotis@6f973c6

[pjrobertson]

I'm familiarizing myself with the QSObject class. Obviously, the commented-out code won't work - as you mention, checking the retain count probably wasn't the right way in the first place. But, what is? :)

Good question, that's what I was getting at with this 'discussion'. Basically that method would run when the interface is closed (and a few other cases) to wipe temporary objects that have been created. In reality, these objects should be wiped once they're not needed again. I.e. they should never be left dangling anywhere.

Your solution looks interesting/good. I never thought about that. But will dealloc ever be called if the objectDictionary is still retaining the object?
If it does work (i.e. you've tested the number of objects in objectDictionary doesn't keep going up), then I think the potentially cleaner fix would be to use [self setIdentifier:nil] in dealloc - this already has the logic of removing the object from the dict.
...no it doesn't. I just checked. It should really, right?
(Ping @tiennou - I know he likes nitty gritty fundamental architecture stuff like this, and always has good ideas :D )

[pjrobertson]

And about your following lines @craigotis - I think the reason some 'arbitrary' retain count of 2 was previously used is because 'important' QSObjects that (typically ones we want to keep around - e.g. that are in the catalog) are retained in a number of places.

I think using Instruments would be a good idea to see where the objects are being retained after closing the interface. If it's just in the objectDictionary, then we kind of have a retain cycle going on. I'll take a look

[skurfer]

I was going to look in more detail later this morning, but my understanding is that the object dictionary is jut an in-memory cache to allow for quick access using objectWithIdentifier: (without doing the work of recreating the object every time). So once the interface is dismissed, it seems like we'd only need to keep things in the catalog, and things in the current result array (in case you call the interface back up without changing your search). But looping over those arrays and the object dict all the time to see what gets removed seems awfully expensive.

[skurfer]

Side-note: The fact that triggers for things not in the catalog work until a restart makes me think those objects were getting kept in the object dictionary somehow, too. We might break that functionality when we fix this, but maybe it's better for people to find out up-front that the trigger isn't viable.

[craigotis]

@pjrobertson You're right, if the objectDictionary is retaining the objects then no, dealloc won't be called.

@skurfer If the object dictionary is an in-memory cache used for quick lookups, then the question of course becomes, when are cache objects invalidated? Without the ability to track retain cycles, I think potential options include:

1. Clear the entire cache when the interface closes. This seems heavy-handed and potentially issue-prone. (Though I don't know how quickly the cache fills up - maybe it's actually very light and quick?)
2. Use an NSTimer to clear the cache 2 minutes after the last registration/access occurred. If you create the cache, then poll it 90 seconds later, the timer is reset to 2 minutes. The downside here is that you're keeping the entire cache atomically, and not examining each cached item individually.
3. Store, with each object/identifier pair, a timestamp indicating when it was registered. In the loop, if the timestamp is older than (current time - 2 minutes) then the item can be removed from the objectDictionary. If the item is still being used somewhere, then whatever is using it will retain the reference to the object until it is no longer needed, even after we've removed it from the cache.

However: I should mention again that my first encounter with this class was about 2 hours ago, so feel free to scrutinize my options/solutions with extreme prejudice. :)

[pjrobertson]

my belief is that cleanObjectDictionary effectively did what 1. in your list above did.
I've just checked and cleanObjectDictionary is only ever called immediately after the interface is closed.

Maybe we could add a BOOL to the QSObject flags (see QSObject.h:65) called shouldCache. For important objects, we set that. For ones we create on the fly, we don't.
Then we loop over objectDictionary and release any that aren't meant to be cached? ...but that doesn't work for things like the results list objects...

[skurfer]

Still haven't looked at any code, so this could be nonsense, but if the things we care about are in the catalog and in the result array, they should be retained, right? So I wonder if it would be possible to make objectWithIdentifier: check those places instead, and just get rid of the object dictionary altogether.

Long term, we should probably be asking QSLibrarian and/or QSRegistry for existing objects, instead of calling a class method on QSObject. Maybe. Going to look at the code now.

[skurfer]

Another side-note: We might have to dig in and actually figure out the bug I fixed with #1566, instead of just hacking around it by putting things back how they were.

[tiennou]

Here's the required "architecture" comment ;-):

• After all those years, I'm still unsure what's the point of that objectDictionary thing, and the main problem I can't think of an easy replacement. We know it's used as an identifier-object cache, and temporary objects end up there, but what was really the point if it's cleaned up as soon as the interface closes ? The example of browsing /usr/bin/ looks like that : you open the folder, children gets loaded and are retained (I think) by some result array somewhere, and when the interface closes they all get released.
• Use NSMapTable (or NSHashStuff, or whatever its name actually is) along with "weak" values. The object gets released from somewhere ? The object dictionary gets automatically cleaned up. No need for a -dealloc override.
• 👍 on the point @skurfer makes on moving that responsibility out to QSLibrarian, since it's the de-facto keeper of the catalog, and we could do some nice things like "we've just loaded and deallocated these temporary items 4 times in a row, maybe the user uses those frequently, let's automatically cache them".

[tiennou]

some 'arbitrary' retain count of 2

The objectDictionary ownership + wherever else.

[craigotis]

@tiennou I agree that the cache seems a weird thing to have, at least in its current form.

@tiennou @pjrobertson Right - the objects weren't removed until their retainCount was less than 2, I assume because this would indicate the cached item was only being referenced by the cache, and not by anything else.

[skurfer]

Just for kicks, I tried changing the test from using retainCount < 2 to using ![[QSLib defaultSearchSet] containsObject:thisObject]. It more or less works and I'm not seeing a penalty as it tries to recreate objects. (The result array seems to keep the objects around on its own.) But that's a complete band-aid. Probably not worth pursuing further.

I was curious about what was getting released. Has anyone tried uncommenting the NSLog() in -[QSObject dealloc] (and change %x to %p)? Looks pretty strange. Lots of stuff is freed on launch, and all of my contacts are deallocated when I first call up the interface (but they work fine).

It also seems that a QSCommand is created every time you change a selection in one of the panes, and then immediately destroyed before you even execute it. Maybe to get the descriptive string?

Who turned over this rock?