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

Rename QSGCD helpers. #1598

Merged
merged 4 commits into from Sep 19, 2013
Merged

Rename QSGCD helpers. #1598

merged 4 commits into from Sep 19, 2013

Conversation

tiennou
Copy link
Member

@tiennou tiennou commented Sep 11, 2013

This makes them inline functions, because they are so simple wrappers it's a pain to imagine all those wasted cycles just doing a queue check and calling libdispatch function.

Please note I've tried to make ol' runOnQueueSync general by using the queue label instead of a dispatch_specific value. I'm not sure how that will impact the scanning process though, since we have one queue per scan process (which IMHO is overkill but, well ;-)). I'll do an Instruments run to check how in behaves.

@tiennou
Copy link
Member Author

@tiennou tiennou commented Sep 11, 2013

Look at that ! Comments ! GitHub, I'm impressed ;-).

Now running Instruments with a cold QS cache (rm -R ~/Library/Caches/Quicksilver-certified ;-)).

  • Wait for it, ~100 queues (!) related to catalog scanning.
  • For reference, it took 1:20 to display the Keychain dialog when the Dropbox plugin.
  • There's a QSCatalogEntry scanQueue: (null) ;-). 5 of them have UUIDs for names (not sure why).
  • The "blocks run" statistic is awe-inducing, queue with most block called is QSPresetRunningProcess with 43 blocks. Average 1, median 2. Here's the array I extracted :
[43, 6, 4, 3, 3, 3, 3, 3, 3, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 2, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]

Other than that, I don't see anything wrong...

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 12, 2013

And I come over to see the comments are still there... OK I'm happy ;-)

I don't really understand your comment though. Is that just after you did some testing? The number of catalog queues hasn't changed after this pull right?

QSCatalogEntry scanQueue: (null) will be caused by a catalog entry that doesn't have an ID. See QSCatalogEntry.m:L76.
Is that a problem? ...I don't know
We should probably track down which ones they are and make sure they have IDs though

dispatch_async(queue, block);
}

inline void QSGCDSync(void (^block)(void))
Copy link
Member

@pjrobertson pjrobertson Sep 16, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these methods say what queue priority they're running on?
QSGCDSyncDefaultPriority... now we're getting long and silly names :P

Copy link
Member Author

@tiennou tiennou Sep 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think using priorities is a Bad Idea™, as explained here. Go read that, I'll wait ;-). Back ? Good, now read the rest of the article, it's very well done :-D.

Joking aside, I don't think having separate "priority" helpers is a good idea (as the article suggests). I'm pretty sure I've seen QS act like that because the main thread is now considered a "resource" because it's the only one that can do UI directly, and thus, firing 200 icon loads (even on a background queue) that will end up updating objects on the main thread synchronously will end badly (as #1607, #1603 seems to imply). An implicit rule I can think of is never dispatch_sync on the main thread things that 1) don't explicitly care about what's returned and 2) — and this is major — dispatch_sync again somewhere else (example here) though it doesn't seem obvious, but look at the stack trace in #1607, for thread 24, yay !).

So if you have no(t too many) objections, I'll consider that merged, and use the deprecated warnings that will show up to take a deep hard look at our deadlock problems, and do a PR with that. Right now I just feel it's too painful to debug anything when you end up deadlocked by something completely unrelated.

Copy link
Member

@pjrobertson pjrobertson Sep 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've read 3 of the articles :)
objc.io looks cool. Although I note nearly all the articles are written by https://github.com/dkduck - I don't know where he gets his info from. Would be nice to have more references and explanations, but it all seems to make sense.

So if you have no(t too many) objections, I'll consider that merged, and use the deprecated warnings that will show up to take a deep hard look at our deadlock problems, and do a PR with that.

Find by me. I've used so many 'bad' (according to the objc.io post) practices it may be difficult… ;-)

P.S. Maybe NASA should have hired Mr Samson to code the rover? :P

On 18 Medi 2013, at 05:36, Etienne Samson notifications@github.com wrote:

In Quicksilver/Code-QuickStepFoundation/QSGCD.h:

+inline void QSGCDQueueSync(dispatch_queue_t queue, void (^block)(void))
+{

  • if (dispatch_queue_get_label(queue) == dispatch_queue_get_label(dispatch_get_current_queue())) {
  •    block();
    
  • } else {
  •    dispatch_sync(queue, block);
    
  • }
    +}

+inline void QSGCDQueueAsync(dispatch_queue_t queue, void (^block)(void))
+{

  • dispatch_async(queue, block);
    +}

+inline void QSGCDSync(void (^block)(void))
I think using priorities is a Bad Idea™, as explained here. Go read that, I'll wait ;-). Back ? Good, now read the rest of the article, it's very well done :-D.

Joking aside, I don't think having separate "priority" helpers is a good idea (as the article suggests). I'm pretty sure I've seen QS act like that because the main thread is now considered a "resource" because it's the only one that can do UI directly, and thus, firing 200 icon loads (even on a background queue) that will end up updating objects on the main thread synchronously will end badly (as #1607, #1603 seems to imply). An implicit rule I can think of is never dispatch_sync on the main thread things that 1) don't explicitly care about what's returned and 2) — and this is major — dispatch_sync again somewhere else (example here) though it doesn't seem obvious, but look at the stack trace in #1607, for thread 24, yay !).

So if you have no(t too many) objections, I'll consider that merged, and use the deprecated warnings that will show up to take a deep hard look at our deadlock problems, and do a PR with that. Right now I just feel it's too painful to debug anything when you end up deadlocked by something completely unrelated.


Reply to this email directly or view it on GitHub.

Copy link
Member Author

@tiennou tiennou Sep 18, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there are lots of cool things in objc.io. I especially like the light-headed approach (for example, the one on not using CoreData, I'll remember to do that when the QSLibrarian will get its rewrite ;-)).

I've already tried to fix some of the deadlocks. If you can keep up with some broken things, I find this one + the deadlock fix + the task-threading pull to be much more "stable".

@skurfer skurfer mentioned this pull request Sep 19, 2013
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Sep 19, 2013

Merging. Although as Rob pointed out it's included in #1611

We're meant to be closing pulls right? ;-)

pjrobertson added a commit that referenced this issue Sep 19, 2013
@pjrobertson pjrobertson merged commit 1ac6db3 into quicksilver:master Sep 19, 2013
@tiennou tiennou deleted the qsgcd-inline branch Jul 20, 2016
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

2 participants