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

ARC migration #1433

Merged
merged 31 commits into from Jul 25, 2013
Merged

ARC migration #1433

merged 31 commits into from Jul 25, 2013

Conversation

craigotis
Copy link
Contributor

@craigotis craigotis commented Mar 20, 2013

This is the first pass of ARC migration for Quicksilver. The goal of this first pull was just to get a build launching that had ARC enabled for at least the Quicksilver app code. (Code-App) These commits have ARC enabled for the main Cocoa product (Code-App) and for QuickStep Interface. There's much work to be done, but this is a start.

pjrobertson and others added 19 commits Mar 6, 2013
allow users to change the global selection trigger
increase build number to 4000

Drinks for everyone!
The latter never existed in Xcode 4. Not sure why this was never an issue before.
…App and Code-QuickStepInterface. Reverted the removal of the release method in NDHotKeyEvent. Altered a few allocWithZone: calls in Code-App, changing them to simple alloc calls.
Fix a few crashes/hangs from general QS use
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 23, 2013

I've just pulled these changes and built - it works!

A few small things:

  • I got errors in VDKQueue about ARC not supporting autorelease, release etc. Strange since VDKQueue is in QuickStep-Core (which you haven't converted to ARC). Adding -fno-objc-arc to the compiler flags of VDKQueue.m fixed this
  • You've manually set the BaseSDK of the Core and Interface targets to 'macosx' (they look bold in the 'Build Settings' pane). We take all the info from our .xcconfig files, so don't want to override them. Just hit ⌫ to go back to the default (which is the same)

My changes are here

I've done a few initial profiles using Instruments and there still seem to be quite a few leaks here and there, and memory usage is higher than before. Still, the benefits on the coding side are obvious and maybe once the whole project is converted things will improve :)

pjrobertson added 2 commits Mar 23, 2013
Otherwise plugins can't build (since the .h file is declared in QSInterface.h)
@craigotis
Copy link
Contributor Author

@craigotis craigotis commented Mar 23, 2013

Glad to hear it! I'm a member of the "commit early, commit often" camp. I hadn't done any memory/leak profiling or Clang analysis yet - just enabled ARC, tweaked enough code to get it launching without crashing, and called it a milestone.

I will update my .xcodeproj to remove the BaseSDK and will take a look at the VDKQueue. My next few commits will focus on improving memory management and plugging any leaks.

Thanks for taking the time and patience to look through the code thus far!

…ng for Debug/Release from the Core and Interface projects.
@craigotis
Copy link
Contributor Author

@craigotis craigotis commented Mar 23, 2013

Somehow I had forgotten to commit the VDKQueue.m/.h changes. Still getting used to git, blame it on user error.

…method was deprecated.

Included a subrepo commit for VDKQueue

Removed an unnecessary release call, and also an unused variable.
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Mar 24, 2013

Somehow I had forgotten to commit the VDKQueue.m/.h changes. Still getting used to git, blame it on user error.

No worries that's fine :)
Watch out for the VDKQueue files, they're a git submodule (run git submodule status or cat .gitmodules to see that) so you can't actually commit them. You need to update the submodule then we just run git submodule update

Basically it means we can just pull the changes straight from https://github.com/bdkjones/vdkqueue
So there are a few ways we can go with this (and also the ND classes which are external - we've been tending not to change them but they're so old now that I think it's fine in this case):

we can either:

1 and 3 are probably the best options.

…-Core project to ignore ARC for the VDKQueue.m file.
@craigotis
Copy link
Contributor Author

@craigotis craigotis commented Mar 24, 2013

Thanks Patrick - I went with option 3. Easy enough since it was a single source file. If we have more substantial changes to VDKQueue in the future, I think option 1 would be more suited, but for now this seems simple enough.

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 25, 2013

If we're going to do 1, there's no reason not to do 2 as well. Can't hurt. But really, as long as VDKQueue isn't abandoned, I say we leave it alone and do 3. I don't see much benefit to ARC if it's code we don't have to maintain.

I've been looking at this as well. The only problem I've run into so far is that the Large Type display disappears almost immediately. I assume the window's getting automatically dealloc'ed at the wrong time.

@craigotis
Copy link
Contributor Author

@craigotis craigotis commented Mar 26, 2013

The only problem I've run into so far is that the Large Type display disappears almost immediately. I assume the window's getting automatically dealloc'ed at the wrong time.

Correct. Creating one-off NSWindow objects and letting them sit on their own is difficult in ARC. Something needs to maintain a strong reference to that window, or it will indeed be immediately deallocated. (And thus closed.)

The committed fix works, and is fairly lightweight, but it would be great if the windows knew somehow to take care of themselves.

@skurfer
Copy link
Member

@skurfer skurfer commented Mar 27, 2013

Yup, that fixes it. I've also tried merging this with several other outstanding pull requests just to see what we're dealing with. None of the conflicts are too hairy, but there are (unsurprisingly) a large number of ARC no-no's that need to be fixed in the other branches before you can build. I'm running with the resulting build for a while just to see how leaky it is and to see if it's introduced any other bugs.

@craigotis
Copy link
Contributor Author

@craigotis craigotis commented Mar 27, 2013

Thanks Rob. A quick comparison last night of the ß71 and a release build of my arc-migration branch with the master 1.0.0 changes merged in, showed the older version sitting around 24MB "Real Mem", and the ARC version sat around 26-27MB.

Tough to say without diving much deeper whether the diference is purely ARC, or any code differences between the master repo and the ß71 I had installed from a couple months ago.

Also, I'm very much looking forward to fixing any further bugs/leaks we find. I'm a fairly light user, generally just a couple plugins with no triggers, but I haven't encountered any leaks yet after running in Instruments for 5-10 minutes.

pjrobertson and others added 2 commits Mar 28, 2013
Instructions to give QS/OS versions
@skurfer
Copy link
Member

@skurfer skurfer commented Jul 25, 2013

For what it's worth, I made a local copy of this branch and rebased against master today. I'm using that as my day-to-day build for now just to see how it goes. I don't think I'm going to submit a pull request for it and I don't think you should necessarily try to update this one (although it can't hurt), since I think it will still be a while before we merge this. We need to get the next release out the door first and I think we're waiting on a couple of small pulls for that.

FYI, when rebasing, if I saw conflicts and it wasn't obvious which version to take, I would just take the version from master and fix any ARC complaints that were there. I've been on this build for a few hours now with no issues.

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 25, 2013

Awesome, good to know you've done the hard rebase work ;-)
I've added quite a few ARC-incompatible things in the past few months [something release] so there were probably quite a few things ARC complained about.

Tbh I was thinking we just merge this perhaps in the next release and send it out for beta testing, since from a code point of view it's OK, it just needs extensive testing.

On 25 Gorff 2013, at 10:31, Rob McBroom notifications@github.com wrote:

For what it's worth, I made a local copy of this branch and rebased against master today. I'm using that as my day-to-day build for now just to see how it goes. I don't think I'm going to submit a pull request for it and I don't think you should necessarily try to update this one (although it can't hurt), since I think it will still be a while before we merge this. We need to get the next release out the door first and I think we're waiting on a couple of small pulls for that.

FYI, when rebasing, if I saw conflicts and it wasn't obvious which version to take, I would just take the version from master and fix any ARC complaints that were there. I've been on this build for a few hours now with no issues.


Reply to this email directly or view it on GitHub.

@craigotis
Copy link
Contributor Author

@craigotis craigotis commented Jul 25, 2013

Thanks Rob. I don't have any additional local modifications that haven't
been checked in.

Things should be stable, as Patrick mentioned it's really just testing at
this point and keeping an eye out for leaks/bugs. If you want to push your
rebase changes somewhere, I can pull them into my local arc-migration
branch also and help test. (And/or update this pull request.)

On Wed, Jul 24, 2013 at 11:12 PM, Patrick Robertson <
notifications@github.com> wrote:

Awesome, good to know you've done the hard rebase work ;-)
I've added quite a few ARC-incompatible things in the past few months
[something release] so there were probably quite a few things ARC
complained about.

Tbh I was thinking we just merge this perhaps in the next release and send
it out for beta testing, since from a code point of view it's OK, it just
needs extensive testing.

On 25 Gorff 2013, at 10:31, Rob McBroom notifications@github.com wrote:

For what it's worth, I made a local copy of this branch and rebased
against master today. I'm using that as my day-to-day build for now just to
see how it goes. I don't think I'm going to submit a pull request for it
and I don't think you should necessarily try to update this one (although
it can't hurt), since I think it will still be a while before we merge
this. We need to get the next release out the door first and I think we're
waiting on a couple of small pulls for that.

FYI, when rebasing, if I saw conflicts and it wasn't obvious which
version to take, I would just take the version from master and fix any ARC
complaints that were there. I've been on this build for a few hours now
with no issues.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1433#issuecomment-21530857
.

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 25, 2013

I had pulled your branch down directly, @craigotis. I was wondering why this didn't show a merge conflict then I saw that it goes into the ARC branch. In that case, I say we go ahead and merge it. Then I can rebase that against master, repeating the changes I did yesterday. Once it's done, you guys can test from that branch. (Actually, it might be much easier to just merge what I did yesterday into ARC if that's OK with everyone.)

I still say we wait until after the next release before merging ARCmaster. So long as ARC is unmerged, it shouldn't be that hard to keep it up to date. Should only be a couple of pull requests that get in before it does.

Sound good?

@craigotis
Copy link
Contributor Author

@craigotis craigotis commented Jul 25, 2013

@skurfer I think that sounds great. The ARC branch should be treated as an unstable testing area anyway.

I also agree with holding off on the merge into master until a little more testing is performed.

Once the merge goes through and your rebased master changes are present in ARC, let me know and I'll pull the latest changes down into my local branch as well.

skurfer added a commit that referenced this issue Jul 25, 2013
@skurfer skurfer merged commit 806399b into quicksilver:ARC Jul 25, 2013
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Jul 25, 2013

Yep, I'm happy with the outline:

  • More testing on the arc branch
  • Release QS 1.0.1 in the meantime
  • Merge arc (if we're happy) into master for 1.0.2 (or 1.1.0...)
  • Lots of beta releases before 1.0.2 goes live :)

@skurfer fine by me if you merge your rebase into arc. It'll make life easier for lazy me (lazy right now at least...) to test out the ARC changes

@skurfer
Copy link
Member

@skurfer skurfer commented Jul 25, 2013

OK, I've rebased and pushed ARC. If you'd pulled it down previously, you'll want to blow it away and pull it down again.

I also added two small commits after the rebase. One to fix some additional ARC problems and one to add @craigotis to the credits. 😃

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

3 participants