Small catalog fixes #1232

Merged
merged 9 commits into from Dec 11, 2012

Conversation

Projects
None yet
3 participants
Owner

pjrobertson commented Nov 20, 2012

Rob slyly got me to do this… :P

Disabling/enabling an item in the catalog prefs then restarting QS wouldn't make the change stick.
This pull fixes that, along with a few other cleanups and GCD

pjrobertson added some commits Nov 20, 2012

Remove unused code
NSTableViewDataSource methods are never called since QSCatalogPrefPane is an NSOutlineViewDataSource
Write the catalog.plist prefs when enabling/disabling a Catalog entry…
… in the prefs

Also:

* Use GCD and get rid of the scanForcedInThread: method
This may conflict with #1143 slightly
Owner

skurfer commented Nov 20, 2012

Rob slyly got me to do this… :P

It would have taken me much, much longer and wouldn't have been nearly as clever. :-)

Looks good. I'll run it for a while.

Owner

skurfer commented Nov 21, 2012

It looks like scanForcedInThread: was responsible for adding and removing catalog scanning tasks in the Task Viewer. Shouldn't that functionality be moved somewhere else?

Owner

skurfer commented Nov 23, 2012

Now here's a crash report for you. I saw this when testing this branch, but that could be coincidence.

Process:         Quicksilver [35096]
Path:            /Applications/Quicksilver.app/Contents/MacOS/Quicksilver
Identifier:      com.blacktree.Quicksilver
Version:         ß71:mine (3936)
Code Type:       X86-64 (Native)
Parent Process:  launchd [142]
User ID:         501

Date/Time:       2012-11-20 15:33:27.539 -0500
OS Version:      Mac OS X 10.8.2 (12C60)
Report Version:  10

Crashed Thread:  7

Exception Type:  EXC_CRASH (SIGABRT)
Exception Codes: 0x0000000000000000, 0x0000000000000000

Application Specific Information:
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[__NSSetM addObject:]: object cannot be nil'
abort() called
terminate called throwing an exception

Application Specific Backtrace 1:
0   CoreFoundation                      0x00007fff9287a0a6 __exceptionPreprocess + 198
1   libobjc.A.dylib                     0x00007fff8d36f3f0 objc_exception_throw + 43
2   CoreFoundation                      0x00007fff9291299c -[__NSSetM addObject:] + 796
3   CoreFoundation                      0x00007fff92857349 -[NSMutableSet addObjectsFromArray:] + 473
4   QSCore                              0x00000001000dc3fe -[QSLibrarian reloadSets:] + 343

…

Thread 7 Crashed:
0   libsystem_kernel.dylib          0x00007fff98118212 __pthread_kill + 10
1   libsystem_c.dylib               0x00007fff8fc8eaf4 pthread_kill + 90
2   libsystem_c.dylib               0x00007fff8fcd2dce abort + 143
3   libc++abi.dylib                 0x00007fff984dba17 abort_message + 257
4   libc++abi.dylib                 0x00007fff984d93c6 default_terminate() + 28
5   libobjc.A.dylib                 0x00007fff8d36f873 _objc_terminate() + 91
6   libc++abi.dylib                 0x00007fff984d93f5 safe_handler_caller(void (*)()) + 8
7   libc++abi.dylib                 0x00007fff984d9450 std::terminate() + 16
8   libc++abi.dylib                 0x00007fff984da5b7 __cxa_throw + 111
9   libobjc.A.dylib                 0x00007fff8d36f50c objc_exception_throw + 327
10  com.apple.CoreFoundation        0x00007fff9291299c -[__NSSetM addObject:] + 796
11  com.apple.CoreFoundation        0x00007fff92857349 -[NSMutableSet addObjectsFromArray:] + 473
12  com.blacktree.QSCore            0x00000001000dc3fe -[QSLibrarian reloadSets:] + 343
13  com.apple.CoreFoundation        0x00007fff9282c47a _CFXNotificationPost + 2554
14  com.apple.Foundation            0x00007fff95df3846 -[NSNotificationCenter postNotificationName:object:userInfo:] + 64
15  com.blacktree.QSCore            0x00000001000d4e33 -[QSCatalogEntry scanAndCache] + 351
16  com.blacktree.QSCore            0x00000001000dd352 -[QSLibrarian loadMissingIndexes] + 245
17  com.blacktree.Quicksilver       0x0000000100008392 0x100000000 + 33682
18  com.apple.Foundation            0x00007fff95e40612 __NSThread__main__ + 1345
19  libsystem_c.dylib               0x00007fff8fc8d742 _pthread_start + 327
20  libsystem_c.dylib               0x00007fff8fc7a181 thread_start + 13

Looking at the code, I don't see how this is possible. Wouldn't the array itself have to contain nil? And you can no more add that to an array than you can to a set, so how would it get there? Maybe one of the objects in the array is being released after its added? But adding to the array should increase the retain count and prevent that.

Owner

skurfer commented Nov 28, 2012

Also, what does this mean for #1143? No longer needed? Should this use a separate queue, as @tiennou does in that branch?

Owner

pjrobertson commented Dec 1, 2012

I think the problem is multi-threading maybe?! :)

I've added an @synchronized() around the block in question, so that might fix it.

Also, it seems like [entry contents] isn't a simple getter, it actually scans and caches the catalog entry every time it's called.
So the three calls to [entry contents] around the problematic lines could all potentially return different things. I've tried to fix this by using a temp array.

Owner

pjrobertson commented Dec 1, 2012

There are now also a couple of commits that add most of what @tiennou changed in #1143.

The changes are:

  • Name the dispatch queue indexQueue. Is indexation a word?!
  • Don't use 10.7+ only methods
  • I haven't done anything with dispatch queues in QSCatalogEntry. I don't think that we should be using the QSLibrarian dispatch queue to block/control catalog entry scanning. They are independent of QSLibrarian, and as such, I think each catalog entry should have its own queue. All scan tasks for each catalog entry will be performed on their own concurrent queue, hopefully avoiding any problems of crashes whilst scanning.

Thoughts?

Owner

skurfer commented Dec 3, 2012

I thought QSLibrarian was nothing more than the aggregated result of all the various catalog scanners, so it doesn't seem out of the question that it should manage/co-ordinate catalog entry scans. But that doesn't mean it needs to. You understand it better than I do, so I'll trust your conclusion.

I've been running with this for a while and had no problems. I'll test it out a bit more, but most likely merge it tomorrow.

Owner

pjrobertson commented Dec 3, 2012

My point about not using QSLibrarian to control scanning was that each QSCatalogEntry is independent, and say the catalog entry for iTunes songs can quite happily be scanning at the same time as say… your Safari bookmarks catalog entry. If we blocked things in QSLibrarian it would make scanning a completely serial process, and a bit slow.

The only time QSLibrarian needs to 'block' anything is when it's writing the catalog… although having said that, I actually don't think it needs to at all. writeCatalog only writes the Catalog.plist file in the app support folder not the .qsindex (cache) files, so the block here (and on Etienne's) branch is probably unnecessary.

It'd still be good if @tiennou had a few seconds to chime in on my previous comments before Rob's merge finger gets fired up ;-)

Owner

skurfer commented Dec 4, 2012

If we blocked things in QSLibrarian it would make scanning a completely serial process, and a bit slow.

OK, I guess I misunderstood how it works. I thought by adding to a queue, you're just piling up things that need to be done, and GCD automatically decides how to do them (possibly concurrently).

Owner

pjrobertson commented Dec 4, 2012

… and GCD automatically decides how to do them (possibly concurrently).

When creating a queue, you decide how you want it to behave (serial or concurrent). 10.6 only supports serial queues (Etienne used 10.7+ methods, hence why one of my changes from Etienne's code (above) is "don't use 10.7+ only methods")
Since we're still stuck on 10.6, the queue is necessarily serial.

Owner

skurfer commented Dec 4, 2012

Well, then I think your approach is correct. I'll leave this open for a while to see if Etienne has anything to say.

Owner

tiennou commented Dec 5, 2012

Tadaaa ! ;-).

All scan tasks for each catalog entry will be performed on their own concurrent queue, hopefully avoiding any problems of crashes whilst scanning.
If we blocked things in QSLibrarian it would make scanning a completely serial process, and a bit slow.

The only thing I don't get is that we were trying to kill mutated-while-being-enumerated exceptions right ? Thrown by QSLibrarian when one of the catalog scanners changed its contents while it was writing that data ? So now I don't get how you can fix those without making scanning completely serial (or at most concurrent with a write barrier which is sadly 10.7+) or putting back @synchronize around things...

Also, I'm wondering about scannerCount. Doesn't that serialize everything anyway ?

Other than that, I don't see anything wrong in the diff, it's just that this scanning process looks messier to me than it should be (which is why I'd tried to bring it back all under QSLibrarian so that it would be easier on the brain).

Owner

pjrobertson commented Dec 5, 2012

Thanks for chiming in Etienne. I'll have more of a think about the mutated-while-being-enumerated exceptions, but does QSLibrarian actually write any data? I think it's all done in QSCatalogEntry. Incase you missed it, here's what I said about writeCatalog:

The only time QSLibrarian needs to 'block' anything is when it's writing the catalog… although having said that, I actually don't think it needs to at all. writeCatalog only writes the Catalog.plist file in the app support folder not the .qsindex (cache) files, so the block here (and on Etienne's) branch is probably unnecessary.


Also, I'm wondering about scannerCount. Doesn't that serialize everything anyway ?

I don't really know what scannerCount does. I'll look into it :)

Here's some fuel for thought, and the strangeness of blocks… Try running this app:

Or, to save you the time, you'll notice that the FIRST enumeration block runs fine, eventhough the array is mutated whilst being enumerated.
The 2nd and 3rd don't.
Not sure if we can use the 1st way somehow to make our code more safe (change every for in loop to use that code).
As a side note - the 1st method returns an NSIndexSet which will subsequently cause a crash if you try and use [arr objectsAtIndexes:ind]. But the interesting thing is that the enumeration doesn't cause a crash.

//
//  main.m
//  EnumerateMutate
//
//  Created by Patrick on 05/12/2012.
//  Copyright (c) 2012 Patrick Robertson. All rights reserved.
//

#import <Foundation/Foundation.h>

int main(int argc, const char * argv[])
{

    @autoreleasepool {

        __block NSMutableArray *arr = [NSMutableArray arrayWithObjects:@"a", @"b", @"c", @"d", @"e", @"f", @"g", @"h", @"i", @"j", @"k", @"l", @"m", @"n", @"o", @"p", @"q", @"r", @"s", @"t", @"u", @"v", @"w", @"x", @"y", @"z", nil];

        NSIndexSet *ind = [arr indexesOfObjectsWithOptions:NSEnumerationConcurrent passingTest:^BOOL(NSString *s, NSUInteger idx, BOOL *stop) {
            if ([s isEqualToString:@"d"]) {
                [arr removeAllObjects];
            }
            return [s isEqualToString:@"o"];
        }];
        NSLog(@"passed test 1");
        arr = [NSMutableArray arrayWithObjects:@"a", @"b", @"c", @"d", @"e", @"f", @"g", @"h", @"i", @"j", @"k", @"l", @"m", @"n", @"o", @"p", @"q", @"r", @"s", @"t", @"u", @"v", @"w", @"x", @"y", @"z", nil];

        NSIndexSet *ind2 = [arr indexesOfObjectsPassingTest:^BOOL(NSString *s, NSUInteger idx, BOOL *stop) {
            if ([s isEqualToString:@"d"]) {
                [arr removeAllObjects];
            }
            return [s isEqualToString:@"o"];
        }];

        NSLog(@"passed test 2");

        arr = [NSMutableArray arrayWithObjects:@"a", @"b", @"c", @"d", @"e", @"f", @"g", @"h", @"i", @"j", @"k", @"l", @"m", @"n", @"o", @"p", @"q", @"r", @"s", @"t", @"u", @"v", @"w", @"x", @"y", @"z", nil];
        NSUInteger newCount = [arr count];
        for (NSString *s in arr) {
            if ([s isEqualToString:@"d"]) {
                [arr removeAllObjects];
            }
            if ([s isEqualToString:@"o"]) {
                NSLog(@"%@",s);
            }
        }

        NSLog(@"passed test 3");
        // insert code here...


    }
    return 0;
}
Owner

pjrobertson commented Dec 10, 2012

OK, to answer @tiennou's discussion of scannerCount… you've put your finger on the one confusing thing about catalog scanning. QSLibrarian attempts to manage the scanning process, but it doesn't actually do the scanning, that's done within the QSCatalogEntry itself (which isn't necessarily serialised)

I'm working on adding serial dispatch queues to each catalog entry as we speak, a commit will be up in a few mins

pjrobertson added some commits Dec 10, 2012

Serial catalog entry scanning
This is a general rework of the changes already mentioned in #1232.
Serial dispatch queues have now been introduced to each Catalog entry. In essence, this means that each catalog entry is responsible for scanning in a serial (single thread) manner, as opposed to trying to get QSLibrarian to do it.
Owner

pjrobertson commented Dec 10, 2012

OK, another few commits pushed. See the commits for info.

One other change not mentioned in the commit is that I've made scanAndCache return void as opposed to an NSArray. No return value was ever being used from this method, so I removed it (I found it confusing, and it would have meant my original GCD idea wouldn't have worked - but I didn't use that idea anyway)

Does this need to be __block? If you were doing isScanning = YES inside the block, I'd say yeah, but since you're calling [self setIsScanning:YES], is it necessary?

Owner

pjrobertson replied Dec 11, 2012

Interesting. I placed this code in the .m file line 516, and the output (see below) is exactly the same with/without using the __block
Tbh I know what __block does, but my ideas seem to be pretty fuzzy around primitive types, iVars, and stuff.

Know any good resources? I don't think there's any real performance loss (just a different memory storage def) so it should be fine either way

...
   int64_t delayInSeconds = 2.0;
   dispatch_time_t popTime = dispatch_time(DISPATCH_TIME_NOW, delayInSeconds * NSEC_PER_SEC);
   dispatch_after(popTime, dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_HIGH, 0), ^(void){
       NSLog(@"outside of scan block: %@",isScanning ? @"YES" : @"NO");
   });
   dispatch_sync(scanQueue, ^{
       NSLog(@"in scan block (pre setIsScanning): %@",isScanning ? @"YES" : @"NO");
       [self setIsScanning:YES];
       NSLog(@"in scan block (post setIsScanning): %@",isScanning ? @"YES" : @"NO");
       sleep(5);
        NSLog(@"in scan block (post sleep(5)): %@",isScanning ? @"YES" : @"NO");
...

Output in both cases:

2012-12-11 18:46:05.430 Quicksilver[1661:303] in scan block (pre setIsScanning): NO
2012-12-11 18:46:05.434 Quicksilver[1661:303] in scan block (post setIsScanning): YES
2012-12-11 18:46:07.431 Quicksilver[1661:5c03] outside of scan block: YES
2012-12-11 18:46:10.436 Quicksilver[1661:303] in scan block (post sleep(5)): YES

Looked over the documentation (again) and you should only need that to make an inherited variable mutable inside the block. You're tests prove that it's being set correctly with or without __block, so I'd say get rid of it …or make it worthwhile and use plain assignment instead of the setter. :-) But probably the first thing.

remove an unrequired __block assignment. Since a setter is used on th…
…e iVar within a block, the __block is unrequired

skurfer added a commit that referenced this pull request Dec 11, 2012

@skurfer skurfer merged commit f2ea4de into quicksilver:release Dec 11, 2012

skurfer added a commit that referenced this pull request Dec 11, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment