CODEWITHSUNDEEP
objective-cmultithreadingthread-safety
GTAE86
GTAE86

Reputation: 1846

Objective C: Better way to wait on thread completion?

In my application I am performing lots of asynchronous I/O on multiple chips on multiple pipes per chip.

Some operations are made up of multiple operations. For example, to read the serial number from one of the chips I must perform two writes and two reads: Write command to check the serial number buffer size, read result. Write command to read serial number value, read result.

Here is the simplified code for that operation:

- (BOOL)readSerialNumber:(NSMutableString*)serialNumber  
{         
  if(nil == serialNumber)  
    return FALSE;  //  Nowhere to store.  

  if(![self sendCommand:GetSerialNumberSize])  
    return FALSE;  

  // Set up some matching event data (not shown), then check it....  

  BOOL matched= FALSE;  
  BOOL timeUp= FALSE;  
  [self setEventWasMatched:FALSE];  
  NSDate* timeUpDate= [[NSDate alloc] initWithTimeIntervalSinceNow:2.0];  
  while((!timeUp) && !matched)  
  {  
   matched= [self eventWasMatched];  // Match state is set from receive code in another thread.  
   timeUp= (NSOrderedDescending != [timeUpDate compare:[NSDate date]]);  
  }  
  [timeUpDate release];  

  NSUInteger serialNumberSize= matchedEvent.serialNumberSize;  
  if(0 == serialNumberSize)  
    return FALSE;  

  if(![self sendCommand:GetSerialNumber ofSize:serialNumberSize])  
    return FALSE;  

  // Set up some matching event data (not shown), then check it....  

  matched= FALSE;  
  timeUp= FALSE;  
  [self setEventWasMatched:FALSE];  
  timeUpDate= [[NSDate alloc] initWithTimeIntervalSinceNow:2.0];  
  while((!timeUp) && !matched)  
  {  
   matched= [self eventWasMatched];  // Match state is set from receive code in another thread.  
   timeUp= (NSOrderedDescending != [timeUpDate compare:[NSDate date]]);  
  }  
  [timeUpDate release];  

  [serialNumber.setString:matchedEvent.serialNumber];  

  return (0 != [serialNumber length]);  
}  

- (void)setEventWasMatched:(BOOL)matched  
{  
  [lockMatch lock];  
  eventMatched= matched;  
  [lockMatch unlock];  
}  

- (void) eventWasMatched  
{  
  BOOL wasMatched= FALSE;
  [lockMatch lock];  
  wasMatched= eventMatched;  
  [lockMatch unlock];  

  return wasMatched;  
}

This code example may not compile or work, but it is a decent representation of code I have working.

There are a couple of things I have questions about here:

Any input would be appreciated. Some might say to use NSOperation/NSOperationQueue or GCD, but trust me, this example is of one of the simple operations. There are others that involve multiple read/write pairs, some one write/multiple reads, etc.

Upvotes: 1

Views: 1057

Answers (2)

Rob
Rob

Reputation: 437432

You asked:

... the Apple Thread Programming Guide states "Although locks are an effective way to synchronize two threads, acquiring a lock is a relatively expensive operation, even in the uncontested case." How can I do that in a more efficient way?

This isn't the source of the inefficiency in your snippet, but if you're wondering about alternatives, refactoring the code to use queues can sometimes help. See Eliminating Lock Based Code in the Concurrency Programming Guide, which says:

Replacing your lock-based code with queues eliminates many of the penalties associated with locks and also simplifies your remaining code. Instead of using a lock to protect a shared resource, you can instead create a queue to serialize the tasks that access that resource. Queues do not impose the same penalties as locks. For example, queueing a task does not require trapping into the kernel to acquire a mutex.

There is also a particularly ingenious GCD solution for controlling access using the reader-writer pattern (use concurrent queue, dispatch_sync reads, dispatch_barrier_async writes; this permits concurrent reads, but serializes them with respect to the writes, which are done asynchronously). Frankly, it's a pattern that probably is not applicable here, but it is discussed in the following videos: WWDC 2011 - Mastering GCD or WWDC 2012 - Asynchronous Design Patterns.

If you're looking for efficiency, the locks aren't the main culprit here, though. It's that while loop. Solve this by using dispatch semaphores or something equivalent, e.g., after sending the request, use:

if(![self sendCommand:GetSerialNumberSize])  
    return FALSE;  

self.semaphore = dispatch_semaphore_create(0);
dispatch_time_t timeout = dispatch_time(DISPATCH_TIME_NOW, (int64_t)(2.0 * NSEC_PER_SEC));
dispatch_semaphore_wait(self.semaphore, timeout);

if (!self.eventWasMatched) {
    // we must have timed out
}

// carry on ...

and the ReadPipeAsync routine can:

obj.eventWasMatched = YES;                // update all the other relevant properties, too
dispatch_semaphore_signal(obj.semaphore); // now inform other process that we had a match

Something like that is likely to be far more efficient.

You go on to ask:

I know from the Allocations instrument that a lot of memory is used to create NSDate objects in my loops that check for the event matching. I cannot have open-ended checks for matching, because it is possible my match criteria will never be met. What is a better way to do it?

Yes, your code would creating a ton of autorelease objects that won't be freed until the pool is drained. Obviously, you should just delete these while loops altogether, but, just for the sake of argument, if you wanted to fix the memory issue here, you could either wrap that in an @autoreleasepool, which lets you drain the pool with greater frequency:

NSDate* timeUpDate= [[NSDate alloc] initWithTimeIntervalSinceNow:2.0];  
while((!timeUp) && !matched)  
{
    @autoreleasepool {   
        matched = [self eventWasMatched];  // Match state is set from receive code in another thread.   
        timeUp  = (NSOrderedDescending != [timeUpDate compare:[NSDate date]]);  
    }
}  
[timeUpDate release];

Or, even better, don't use an autorelease object at all:

CFAbsoluteTime startTime = CFAbsoluteTimeGetCurrent();
while((!timeUp) && !matched)
{
    matched = [self eventWasMatched];  // Match state is set from receive code in another thread.
    timeUp  = ((CFAbsoluteTimeGetCurrent() - startTime) >= 2.0);
}

But, as I said earlier, that's really a moot issue, as you should replace these while loops with a more efficient interprocess communication, such as dispatch semaphores.

Upvotes: 1

JeremyP
JeremyP

Reputation: 86651

I know the NSLock access in the set/get functions for the BOOL is expensive.

How do you know that? Bear in mind that you are reading and writing IO devices. In comparison a lock is going to be minuscule (unless your IO channels are of similar speed to the memory bus).

I know the hand-made "sleep" with the timeup check is probably costly

I don't see any sleeps, only a couple of tight loops. They are certainly going to take 100% of one CPU core while running.

It's difficult to say how to improve this, you don't really show enough code. You could do it with NSOperations, e.g. you would have an operation for the write and an operation for each required read and you'd use dependencies to make sure they got executed in the right order, but, assuming that reading and writing the devices is done by reading and writing file handles, I would use a run loop and NSFileHandles.

Upvotes: 0

Related Questions