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

fix the object dictionary #1648

Merged
merged 10 commits into from Dec 5, 2013
Merged

fix the object dictionary #1648

merged 10 commits into from Dec 5, 2013

Conversation

skurfer
Copy link
Member

@skurfer skurfer commented Oct 14, 2013

Here’s what I was suggesting for #1625. The commits should be pretty clear. I could be completely wrong, but I thought I’d get this out there for discussion. It surely conflicts with some other open pulls.

So what will be different? In practice, I haven’t seen any difference, but there’s a significant theoretical difference: Temporary items (not in the catalog, but created in real-time by browsing or running an action) are no longer added to the object dictionary. It follows that…

  1. We no longer need to “clean up” the object dictionary.
  2. Triggers and other commands that used to break after a relaunch will be broken immediately upon creation instead. (That’s an improvement, in my book. More consistent. Less confusing and surprising.) But I haven’t tested that this actually happens.

One of my concerns was that if you browse into a huge folder, dismiss the interface, then call it back up to look at something else in the same list of results, something would break. But it looks like the correct thing is happening. That is, the result array holds onto the objects, so they are still available when you call the interface back up. If you do another search which updates the result array, all those old objects get deallocated immediately. (This is based on informal testing where I just watched the memory usage.)

The only thing I don’t like is that QSObject has some dependencies on QSLibrarian. I think we could work around it by posting notifications for most things, but I couldn’t see a way around it in [QSObject parent].

Long term, all of the plug-ins would need to be updated to call -[QSLib objectWithIdentifier:]. Once that’s done, I have a crazy dream where -[QSObject objectWithIdentifier:] will do what makeObjectWithIdentifier does now (because I never get that right the first time).

Be sure to check out that first (mostly unrelated) commit. A class importing categories on itself had a bit of a smell to it, so I tried removing those. Then I just kept going.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 14, 2013

Interesting, looks good.

About -[QSObject parent]: perhaps the way to go is to just set the parent object explicitly in the cache dict and get rid of the silly parent ID meta. Why save the ID if we can just save the whole object? This'll fix things where a parent doesn't have an ID (most notable: right arrowing into a string with paragraphs. Right now trying to ← just doesn't work)
So here's my idea:

  • in -[QSSOV browse:] we do [someChildObject setParent:parent]
  • This calls through to -[QSObject setObject:parent forKey:kQSParentKey]
  • In -[QSObject dealloc] we are sure remove the parent key (I think setting cache = nil may already do this with ARC, not sure)

About the calls to QSLib in QSObject, maybe -[QSObject objectWithIdentifier:] should be marked as deprecated and that method should be moved to QSLib? Also, I think QSLib should manage setting the identifier of the object. Basically, move all the setIdentifier: logic to QSLib and make the method in QSObject just a simple setter/getter.
That brings up the question of: why do we ever lazy load an object's identifier. Shouldn't we do it in the init method? (my guess is that's probably too soon to do so though)

As I said I'm a bit ill atm, I'll try and take a look in the next few days.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Oct 14, 2013

P.S.:

findDuplicateOrRegisterID - just... yuck! Isn't the whole point of objectWithIdentifier: to avoid the chance of ever getting duplicates?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Oct 15, 2013

About -[QSObject parent]: perhaps the way to go is to just set the parent object explicitly in the cache dict and get rid of the silly parent ID meta. Why save the ID if we can just save the whole object?

Makes sense. But then why even add a layer of complexity with the cache. Why not just set parent as a weak property? (I guess it depends on how common parentOfObject is).

About the calls to QSLib in QSObject, maybe -[QSObject objectWithIdentifier:] should be marked as deprecated and that method should be moved to QSLib?

I already did both of those things, so… agreed! 😉 But so many plug-ins call it on QSObject, so for now it needs to remain a pass-through to the QSLib version.

Also, I think QSLib should manage setting the identifier of the object. Basically, move all the setIdentifier: logic to QSLib and make the method in QSObject just a simple setter/getter.

Well, but when the identifier is set on a QSObject, the object dictionary needs to be updated, since it indexes things by identifier. I was thinking of solving that by having -[QSObject setIdentifier:] post a notification that QSLibrarian watches for. That way, it wouldn't have to directly use QSLib.

Or maybe we just accept the assumption that an instance of QSLibrarian will always be available and talk to it directly whenever we feel like it. 😃

That brings up the question of: why do we ever lazy load an object's identifier. Shouldn't we do it in the init method? (my guess is that's probably too soon to do so though)

I don't know. To save memory?

As I said I'm a bit ill atm, I'll try and take a look in the next few days.

Hope you feel better soon.

findDuplicateOrRegisterID - just... yuck! Isn't the whole point of objectWithIdentifier: to avoid the chance of ever getting duplicates?

Yeah, I don't know. It seems unnecessary. But looking at it makes me wonder if we were supposed to be using that in all those places we currently use objectWithIdentifier. Oh, well.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Nov 6, 2013

Can one of the admins verify this patch?

@tiennou
Copy link
Member

@tiennou tiennou commented Nov 6, 2013

@HenningJ Did you get a comment frenzy, like right now ? You've just commented on every pull request open in under 3 seconds ;-).

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Nov 6, 2013

Ups. That wasn't me.

Or at least not directly. I'm playing around with setting up a build server. You know, making sure QS still builds after every commit and for every PR. I'll elaborate once it's up and running

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Nov 6, 2013

add to whitelist
(still testing)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 13, 2013

OK so one thing we know for sure is that all of this is a mess. Any changes we make will just be moving that mess, but this is certainly moving the mess into an ARC-compatible pile.

Do you think you could do a few final things (if you agree with them of course) so we can push this out the door (and hopefully v1.2?)

  • Store the actual parent QSObject instead of doing fancy things with identifiers and what not. Seems like much less hassle. (I don't think you want to make it weak either, you want strong otherwise if the object gets released, then the parent will be dangling - nobody likes dangling parents)
  • Move findDuplicateOrRegisterID� to QSLib. Seems like it should belong there now. I guess a nicer method name (is this nicer?!) like registeredObjectForObject:(QSObject *)newObject could be in order as well :P

Interestingly, the only place I see where a trigger is broken is for a Menu bar item (e.g. Safari ⇥ Menu Bar Item... ⇥ New Tab), but that was broken anyway. All looks good to me...
(P.S. I still think lazy loading identifiers seems strange to me, but I think that's probably outside the scope of this pull

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 13, 2013

Oh, and I guess this is ok to test now then :)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 13, 2013

Store the actual parent QSObject instead of doing fancy things with identifiers and what not. Seems like much less hassle. (I don't think you want to make it weak either, you want strong otherwise if the object gets released, then the parent will be dangling - nobody likes dangling parents)

Sure. But if the parent has a reference to all the children and the children have a (strong) reference to the parent, isn’t that a textbook retain cycle?

Move findDuplicateOrRegisterID to QSLib.

How about we let that be Plan B? I’d like to just get rid of it if we can.

skurfer added 2 commits Nov 13, 2013
This eliminates a dependency on QSLibrarian.
As of dbe8512, objects will always be created with a "registered"
identifier.
@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 13, 2013

OK, see what you think now.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 13, 2013

Sure. But if the parent has a reference to all the children and the children have a (strong) reference to the parent, isn’t that a textbook retain cycle?
Took me a minute to figure out, but yeah I think you’re right.
The thing is, we don’t really want to use a weak reference. Here’s my use case:

  • → into aParent (which is currently retained by resultsArray)
  • Load the children, changing resultsArray. aParent is released.
  • Try to hit ←: crash because aParent (referenced from the child) is now garbage

Maybe the thing doesn’t actually crash because somewhere else is holding onto aParent.
…maybe this is why only the identifier was ever stored.

On 13 Nov 2013, at 22:32, Rob McBroom notifications@github.com wrote:

Store the actual parent QSObject instead of doing fancy things with identifiers and what not. Seems like much less hassle. (I don't think you want to make it weak either, you want strong otherwise if the object gets released, then the parent will be dangling - nobody likes dangling parents)

Sure. But if the parent has a reference to all the children and the children have a (strong) reference to the parent, isn’t that a textbook retain cycle?

Move findDuplicateOrRegisterID to QSLib.

How about we let that be Plan B? I’d like to just get rid of it if we can.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 13, 2013

Maybe the thing doesn’t actually crash because somewhere else is holding onto aParent.

Well, in most cases, the catalog/“object dictionary” will have it. But I’ve tested arrowing back out to something I know isn’t in the catalog and it seems fine. I think the parentStack mentioned in the browse method might have it. History, too.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 13, 2013

OK, but you still need to be very careful with weak references. They just become garbage when released, so if a child object tries to access a released parent things will go wonky.
It may be worth commenting out the history and parentStack code to see what happens… :D

On 14 Nov 2013, at 00:02, Rob McBroom notifications@github.com wrote:

Maybe the thing doesn’t actually crash because somewhere else is holding onto aParent.

Well, in most cases, the catalog/“object dictionary” will have it. But I’ve tested arrowing back out to something I know isn’t in the catalog and it seems fine. I think the parentStack mentioned in the browse method might have it. History, too.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 13, 2013

It may be worth commenting out the history and parentStack code to see what happens

I commented the lines that add things to them (so they’d always be empty). No crash. It just refuses to go back.

I fixed a warning just now, too. Make sure it still looks right.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 14, 2013

Sure. But if the parent has a reference to all the children and the children have a (strong) reference to the parent, isn’t that a textbook retain cycle?
Took me a minute to figure out, but yeah I think you’re right.
The thing is, we don’t really want to use a weak reference. Here’s my use case:

  • → into aParent (which is currently retained by resultsArray)
  • Load the children, changing resultsArray. aParent is released.
  • Try to hit ←: crash because aParent (referenced from the child) is now garbage

Maybe the thing doesn’t actually crash because somewhere else is holding onto aParent.
…maybe this is why only the identifier

On 13 Nov 2013, at 22:32, Rob McBroom notifications@github.com wrote:

Store the actual parent QSObject instead of doing fancy things with identifiers and what not. Seems like much less hassle. (I don't think you want to make it weak either, you want strong otherwise if the object gets released, then the parent will be dangling - nobody likes dangling parents)

Sure. But if the parent has a reference to all the children and the children have a (strong) reference to the parent, isn’t that a textbook retain cycle?

Move findDuplicateOrRegisterID to QSLib.

How about we let that be Plan B? I’d like to just get rid of it if we can.


Reply to this email directly or view it on GitHub.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 15, 2013

Oh, and this should be OK to test anyway, although I haven't checked the latest commits...

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 20, 2013

Unscientific observation based on memory alone: Quicksilver is using more memory after the changes to the way parent is stored. I think it typically uses around 185MB on my system and has ben consistently around 250MB lately.

I see a few leaks in Instruments, but no more than I see on master. (Not that I really know what I’m looking at.) So I think it might be legitimately hanging onto more objects.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 20, 2013

Ran across something in my reading today that makes me think your worries about crashing are unfounded:

__weak specifies a reference that does not keep the referenced object alive. A weak reference is set to nil when there are no strong references to the object.

So in the scenario you were thinking of, it’ll be nil, and not some random memory address.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 21, 2013

Hmmm.... maybe it was 10.6 that __weak references aren't set to nil for.
Now you mention it I think ARC apps set __weak to nil, which would
make this fine.

What happens to memory usage if you use __weak instead? I think the
main problem is the memory usage increase - maybe we'll have to go
back to how it was? :(

On 21/11/2013, Rob McBroom notifications@github.com wrote:

Ran across something in my reading today that makes me think your worries
about crashing are unfounded:

__weak specifies a reference that does not keep the referenced object
alive. A weak reference is set to nil when there are no strong
references to the object.

So in the scenario you were thinking of, it’ll be nil, and not some random
memory address.


Reply to this email directly or view it on GitHub:
#1648 (comment)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 21, 2013

What happens to memory usage if you use __weak instead?

Use it how? I think when you add the (weak) attribute, it just tells the compiler to insert __weak when declaring the iVar.

I think the main problem is the memory usage increase - maybe we'll have to go back to how it was? :(

You mean before ARC? Or you mean leave master like it is now? Wasn’t that guaranteed to waste lots of memory? I’ve seen a fair amount of improvement since fixing that leak yesterday. I’ll try to compare the leaks branch alone to the leaks branch with this change to see if I’m just imagining the increase.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Nov 22, 2013

Apologies for my last email’s brevity. It was sent whilst I was on a 40 hour train ride using Lynx and a poor 2G connection ;-)

What I meant about ‘going back to how it was’ was referring to the ‘parent’ property; i.e. just go back to only saving the identifier like it was before. This pull req should still be merged, just the way parent is stored needs adjusting.

I’ll look into your leaks branch today, I promise :)

On 21 Nov 2013, at 23:19, Rob McBroom notifications@github.com wrote:

What happens to memory usage if you use __weak instead?

Use it how? I think when you add the (weak) attribute, it just tells the compiler to insert __weak when declaring the iVar.

I think the main problem is the memory usage increase - maybe we'll have to go back to how it was? :(

You mean before ARC? Or you mean leave master like it is now? Wasn’t that guaranteed to waste lots of memory? I’ve seen a fair amount of improvement since fixing that leak yesterday. I’ll try to compare the leaks branch alone to the leaks branch with this change to see if I’m just imagining the increase.


Reply to this email directly or view it on GitHub.

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 22, 2013

I don’t know what to think. I’ve been running with this, combined with the leak fix and it seems to have gone back to around 180 MB over a day or so of running. Unfortunately, I think the only way to know is to let it run for a while.

Here are some numbers I got by doing the same tasks with different branches, but of course QS only runs for a minute or two in these tests:

branch memory
master 158.2 MB
leaks 160.7 MB
leaks + lookup 161.9 MB

@skurfer
Copy link
Member Author

@skurfer skurfer commented Nov 29, 2013

Merged master here to get the shared schemes, and I also put the parent thing back the way it was. I don’t think it makes much difference for memory footprint, but I figured out why it was the way it was: By keeping the parent ID only in a dictionary, the object can be archived as a plist and recreated later without losing the pointer to its parent.

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Nov 29, 2013

ok to test (testing tests :-))

@HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Dec 3, 2013

ok to test

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 5, 2013

By keeping the parent ID only in a dictionary, the object can be archived as a plist and recreated later without losing the pointer to its parent.

Good shout.
Is this the only thing holding up our first ARC release (which should probably be a dev preview for a while)?

pjrobertson added a commit that referenced this issue Dec 5, 2013
@pjrobertson pjrobertson merged commit 193b0b1 into master Dec 5, 2013
1 check passed
@pjrobertson pjrobertson deleted the lookup branch Dec 5, 2013
@skurfer
Copy link
Member Author

@skurfer skurfer commented Dec 5, 2013

Is this the only thing holding up our first ARC release (which should probably be a dev preview for a while)?

Yes it is, and yes it should. I’ll try to get one out tomorrow.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 5, 2013

Cool! :)

On 5 Rhag 2013, at 10:33, Rob McBroom notifications@github.com wrote:

Is this the only thing holding up our first ARC release (which should probably be a dev preview for a while)?

Yes it is, and yes it should. I’ll try to get one out tomorrow.


Reply to this email directly or view it on GitHub.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 5, 2013

...And goodnight.

On 5 Rhag 2013, at 10:37, Patrick Robertson robertson.patrick@gmail.com wrote:

Cool! :)

On 5 Rhag 2013, at 10:33, Rob McBroom notifications@github.com wrote:

Is this the only thing holding up our first ARC release (which should probably be a dev preview for a while)?

Yes it is, and yes it should. I’ll try to get one out tomorrow.


Reply to this email directly or view it on GitHub.

@tiennou
Copy link
Member

@tiennou tiennou commented Dec 5, 2013

Shouldn't plugins be updated to ARC too ? Right now (since the ARC setting was hardcoded in Quicksilver.xcodeproj), only the main app & frameworks are ARC-ified.

EDIT: At least core plugins I meant. The rest can wait a little...

@skurfer
Copy link
Member Author

@skurfer skurfer commented Dec 5, 2013

At least core plugins I meant.

I didn’t realize they weren’t. Yes.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Dec 6, 2013

I don't think it's that urgent. Nice to have, but not a necessity?

We could get an ARC release out now for testing, whilst we ARC-ify the plugins as well. @craigotis what was your initial idea for ARC-ifying Quicksilver? Did you just to the core/frameworks at first so we could take baby steps and test things out as we went?

@craigotis
Copy link
Contributor

@craigotis craigotis commented Dec 6, 2013

My original goal was only to ARC-ify the app and framework targets, as a good starting point. But yes, the rest of the core plugins should be updated to use ARC, at some point in the near future. I can get started on this.

There's no issue with releasing a new build with only certain targets ARC-enabled. It's really a matter of personal preference. On the one hand, you can ARC-ify the entire app in one fell swoop, and "Get it done." On the other hand, a two-stage release, where the app/frameworks are ARC-enabled first, makes it a little easier to get a hold on any early bugs or leaks.

For easy-viewing reference, the selected items are ARC-enabled:

screen shot 2013-12-06 at 8 18 38 am

@craigotis
Copy link
Contributor

@craigotis craigotis commented Dec 7, 2013

Hey guys - I pushed up a new branch, arc-plugins, here:

https://github.com/quicksilver/Quicksilver/tree/arc-plugins

It has ARC-ified versions of the 5 .qsplugin projects. Feel free to take a look! (It was all in one commit, so you can just skip a step and view the diff here: 42f579e)

@skurfer
Copy link
Member Author

@skurfer skurfer commented Dec 9, 2013

I’d say go ahead with a pull request for that guy. I’m running with it now to see how it goes.

@craigotis
Copy link
Contributor

@craigotis craigotis commented Dec 10, 2013

Thanks @skurfer, pull request created! #1710

I'm assuming master was the appropriate target branch?

@skurfer
Copy link
Member Author

@skurfer skurfer commented Dec 10, 2013

I'm assuming master was the appropriate target branch?

Correct. As additional things get merged, I’m just rebasing release from master and updating the release notes. When the release becomes final, we’ll tag it and merge release into master.

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

Successfully merging this pull request may close these issues.

None yet

5 participants