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:

https://github.com/craigotis/Quicksilver/commit/6f973c6a9b07d3ae8cf064279176872efb6712de

[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.
• :+1: 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?

[pjrobertson]

Interesting idea - I see what you're trying to do: "if it's not in the catalog then remove it". Won't this break triggers? I guess you said it's not worth pursuing as you say… I'm stumped for any real solutions though. I like Etienne's idea of adding QSObjects to an NSDictionary (hash table) 'weakly' so that if that's the only place they're left they'll get blown away with the wind. Now how to implement?

Who turned over this rock?

Dunno. Whoever did, feel free to shoot him ;-)

On 2 Oct 2013, at 22:14, Rob McBroom notifications@github.com wrote:

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?

— Reply to this email directly or view it on GitHub.

[tiennou]

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?

Because QSCommand looks like a late addition to the framework. Initially everything was done "in parts", (direct, action, indirect), and some parts really feel that way. I've been trying to consolidate that a little, but as I pointed out in the dev group, I'd really prefer not to go anywhere near QSObjectView & QSInterfaceController while still sober.

Ideally QSInterfaceController should have a QSCommand ivar, its 3 search object views should have their object value bound to each QSCommand ivar. You can dig up the QSCommandInterfaceController in alcor's branch if you want to see "the future" ;-).

Interesting idea - I see what you're trying to do: "if it's not in the catalog then remove it". Won't this break triggers? I guess you said it's not worth pursuing as you say…

It shouldn't, because temporary objects in triggers are archived commands, which contains either an id or a complete archive to recreate the actual object, and this is retained by the command, so it's expected to go away. Unless something has got an ID that's more complex than text or a file path ;-).

I like Etienne's idea of adding QSObjects to an NSDictionary (hash table) 'weakly' so that if that's the only place they're left they'll get blown away with the wind. Now how to implement?

[NSMapTable strongToWeakObjectsMapTable] ;-). But we're still missing one piece of the puzzle : what was the point of that cache anyway ? If we move it to NSMapTable, it won't work anymore like it did because the current version — which I'm labeling as "buggy" because it clears its cache on interface close — was time-based. There's no other way to do that than how it's done (unless maybe using NSCache, and moving the time-based expiration function in there).

[tiennou]

NSCache won't do.

[pjrobertson]

NSCache won't do.

Lol :)

Maybe both you and Rob are right… what is the point of the cache? The only thing I can think of is duplicate objects:

We have a "/Applications" QSFileObject in the Catalog. We browse to / → which creates objects on the fly. If we do no checking to see if an "/Applications" QSFileObject already exists, QS will create a new one Suppose we do something with that (newly created) QSFileObject, like open it. It'll be added to the "Recent Objects" catalog entry, hence adding a dupe: two objects, same path, different memory pointer

On 2 Oct 2013, at 22:30, Etienne Samson notifications@github.com wrote:

NSCache won't do.

— Reply to this email directly or view it on GitHub.

[skurfer]

Maybe both you and Rob are right… what is the point of the cache?

I’ve always thought it was important, but now I wonder. Patrick is right about the filesystem browsing example. Recreating files is expensive. I’ve added this line to -[QSObject objectWithIdentifier:] to see if it’s really doing much.

NSLog(@"object with identifier %@", anIdentifier);

Feel free to do the same and see what you think. My inclination is to say that we need a lookup table like this. (Just not the one we have.)

[tiennou]

Welcome to mess-land ;-). Frankly, this is killing my brain. I've stormed around /usr/ for about 5 minutes :

• I loaded the 11531 entries from /usr/share/man/man3.
• Browsed around /usr/local/include/boost/** into various directories.

Memory usage was all nice and clean (I'm seeing a surge from 497MB to 512MB when I do that, but it cleans itself up when the interface closes).

Here's what I can see about the objectDictionary cache : Objects are checked for existence in -parent if the handler isn't able to provide a parent, -objectWithIdentifier:, and -registerObject:withIdentifier: (which is only called for QSFileObjects, and -findDuplicateOrRegisterID which only happens on deserialization). Ergo, there are many places that put objects in there, but not that many that check the cache (if you agree with me that -objectWithIdentifier: isn't used that often).

All in all, I'm thinking that maybe the iconLoadedSet and childLoadedSet are more useful than this to tackle temporary objects...

On another note, I'm really wondering why we aren't asking the catalog for that identifier first (that's the point @pjrobertson made above about browsing from / to /Applications) ?

[skurfer]

and -registerObject:withIdentifier: (which is only called for QSFileObjects

I noticed that, too. I think that should be changed to setIdentifier:.

On another note, I'm really wondering why we aren't asking the catalog for that identifier first (that's the point @pjrobertson made above about browsing from / to /Applications) ?

Good point. Checking the catalog for an existing object will probably eliminate most of the expensive re-creation scenarios.

But currently, QSLibrarian only has a flat set of objects. We'd have to either enumerate to find an object with a particular ID (too slow), or create something like objectDictionary over there (which I think we all loosely agree on). But I don't think we want QSObject depending on an instance of QSLibrarian, so maybe have QSObject post a notification whenever an identifier is changed so QSLibrarian can keep the index up to date? For the parent thing, maybe we should have a weak reference to the parent object, instead of an identifier in the metadata dict?

The other stuff, like objectWithIdentifier: and findDuplicateOrRegisterID should be in QSLibrarian (though we'd need to take a good look at plug-ins using those), and registerObject:withIdentifier: can just be removed, I think.

[pjrobertson]

You guys... who needs all this chat when you can just use NSMapTable like Etienne said?

It seems to work. See bba31e65a21d91b5c89c48e4f540aa2a040c2b53

I'm not too sure what Etienne was saying about needing the thing to be time based? It all seems to work fine - the relevant places hold on to the objects (the results list, etc) and so they stay in the objectDict.

[pjrobertson]

Oh I know what's wrong with this... the NSPointerFunctionsWeakMemory (which makes the objects NULL on last release) is 10.8+ only. The other option - NSPointerFunctionsZeroingWeakMemory will hold a non-retained pointer. How can we deal with 10.7?

[pjrobertson]

…time to drop 10.7 support? :)

On 3 Oct 2013, at 03:35, Rob McBroom notifications@github.com wrote:

and -registerObject:withIdentifier: (which is only called for QSFileObjects

I noticed that, too. I think that should be changed to setIdentifier:.

On another note, I'm really wondering why we aren't asking the catalog for that identifier first (that's the point @pjrobertson made above about browsing from / to /Applications) ?

Good point. Checking the catalog for an existing object will probably eliminate most of the expensive re-creation scenarios.

But currently, QSLibrarian only has a flat set of objects. We'd have to either enumerate to find an object with a particular ID (too slow), or create something like objectDictionary over there (which I think we all loosely agree on). But I don't think we want QSObject depending on an instance of QSLibrarian, so maybe have QSObject post a notification whenever an identifier is changed so QSLibrarian can keep the index up to date? For the parent thing, maybe we should have a weak reference to the parent object, instead of an identifier in the metadata dict?

The other stuff, like objectWithIdentifier: and findDuplicateOrRegisterID should be in QSLibrarian (though we'd need to take a good look at plug-ins using those), and registerObject:withIdentifier: can just be removed, I think.

— Reply to this email directly or view it on GitHub.

[tiennou]

What's the general behavior if you just leave it out ?

[pjrobertson]

the number of objects being stored in objectDictionary goes up and up and up.

FYI, here's the %age of requests to qs0.qsapp.com in September, by OS

OS	%age requests to qs0.qsapp
10.5	0.0
10.6	5.7
10.7	14.1
10.8	77.2
10.9	3.1

[tiennou]

Le 6 oct. 2013 à 03:03, Patrick Robertson notifications@github.com a écrit :

the number of objects being stored in objectDictionary goes up and up and up.

I meant "if you just remove it entirely" ;-). Is the performance hit that impressive ?

[pjrobertson]

See https://github.com/quicksilver/Quicksilver/issues/1625#issuecomment-25539944

If we remove it all together (or do as I just did - comment out QSObject.m lines 147, 713 and 727) you get dupes in your catalog

[tiennou]

Hmm, then the only other solution I can think of is move the objectDictionary code in a Category and disable ARC in it so we can still access the retainCount and handle memory manually...

[pjrobertson]

Yeah not a bad ideas. I looked into using MAWeakReferencing (look for it on GH, sorry in an iPod atm) and it's work with 10.7 but is non ARC so it's either use that non-ARC lib or our own. If we're not going to haul port 10.7 for too much longer then it's an easy temp fix

On 6 Hyd 2013, at 17:51, Etienne Samson notifications@github.com wrote:

Hmm, then the only other solution I can think of is move the objectDictionary code in a Category and disable ARC in it so we can still access the retainCount and handle memory manually...

— Reply to this email directly or view it on GitHub.

[pjrobertson]

P.S. Just read that. I knew iOS autocorrect was bad, but not THAT bad. Apologies

On 6 Hyd 2013, at 19:27, Patrick Robertson robertson.patrick@gmail.com wrote:

Yeah not a bad ideas. I looked into using MAWeakReferencing (look for it on GH, sorry in an iPod atm) and it's work with 10.7 but is non ARC so it's either use that non-ARC lib or our own. If we're not going to haul port 10.7 for too much longer then it's an easy temp fix

On 6 Hyd 2013, at 17:51, Etienne Samson notifications@github.com wrote:

Hmm, then the only other solution I can think of is move the objectDictionary code in a Category and disable ARC in it so we can still access the retainCount and handle memory manually...

— Reply to this email directly or view it on GitHub.

[skurfer]

I know the answer to this question changes nothing, but why is retainCount an illegal call in the first place? Managing them is automatic, but they still exist. I don’t see why you’re not allowed to even ask what it is.

The category sounds like an OK idea. Could we just add something that returns retain count to the existing category on NSObject (and disable ARC for that) so we can use it anywhere? Or will that not fool the compiler?

[craigotis]

It's an illegal call because, even though you're allowed to ask (via a category), doing so would indicate you plan on acting on that information, which is what ARC is trying to avoid.

The issue isn't specifically that ARC doesn't want you to be calling retainCount, it's that ARC doesn't want you to be manually managing memory, which using retainCount almost guarantees you're going to try to do. (And ARC wouldn't be wrong in this case, would it? ;-) )

I think that the idea of removing an item from a cache, once it's only referenced by the cache, is inherently a code smell when you're using ARC. I think The Right Solution™ is to just drop the cache once the interface closes. Unless I'm mistaken, and I might be, isn't the interface the only thing that relies on it? Once you drop the cache, the items it was maintaining will automatically be purged as well.

[skurfer]

Yeah, you're probably right about all of that.

I think The Right Solution™ is to just drop the cache once the interface closes. Unless I'm mistaken, and I might be, isn't the interface the only thing that relies on it?

Not exactly. I think the interface just needs to make sure the objects in the result array stay around (and I think that already happens independent of this cache).

There are other places that rely on the cache, but I think every single one is just getting an object that it expects to exist in the catalog. That goes back to what's already been said: We should be able to actually ask the catalog (QSLib) for these things instead of relying on the side-effect that everything in the catalog is also in this cache on QSObject. I think we could get rid of the cache altogether at that point.

Should I take a crack at it? Is anyone working on this?

[pjrobertson]

Should I take a crack at it? Is anyone working on this?

Short of what I've done with the NSMapTable; nope. Feel free to go ahead :)

On 8 Oct 2013, at 03:09, Rob McBroom notifications@github.com wrote:

Yeah, you're probably right about all of that.

I think The Right Solution™ is to just drop the cache once the interface closes. Unless I'm mistaken, and I might be, isn't the interface the only thing that relies on it?

Not exactly. I think the interface just needs to make sure the objects in the result array stay around (and I think that already happens independent of this cache).

There are other places that rely on the cache, but I think every single one is just getting an object that it expects to exist in the catalog. That goes back to what's already been said: We should be able to actually ask the catalog (QSLib) for these things instead of relying on the side-effect that everything in the catalog is also in this cache on QSObject. I think we could get rid of the cache altogether at that point.

Should I take a crack at it? Is anyone working on this?

— Reply to this email directly or view it on GitHub.

[skurfer]

OK. I've done something and it seems to be working. I want to test it out a bit (and I'll need to redo it in a cleaner way) before sharing.

[pjrobertson]

Awesome, nice work :)

On 9 Oct 2013, at 22:01, Rob McBroom notifications@github.com wrote:

OK. I've done something and it seems to be working. I want to test it out a bit (and I'll need to redo it in a cleaner way) before sharing.

— Reply to this email directly or view it on GitHub.

[pjrobertson]

Fixed now the #1648 is merged. We still need to do logs of pre-release testing though