Using Multiple images (and threads)

[Benjamin Gilbert]

On 07/17/2014 05:12 AM, Derek Magee wrote:
> The solution I’ve been using is to close the images, however
> openslide_close as documented is not thread safe, so I wrap everything
> up with mutexes:
>
> if(!check_openslide()) return false ;
>
>      m_doing_openslide_operation=true ;
>
> // Some thread safe openslide operation
>
>      m_doing_openslide_operation=false ;

As John said, you have a huge mess here.  A thread can clear 
m_doing_openslide_operation while another thread is still executing in 
OpenSlide, and you don't wait for m_doing_openslide_operation to be 
cleared before starting a close().  More generally, you don't atomically 
test-and-set on your lock paths, and you don't have the memory barriers 
necessary to propagate updated values to other CPUs.  Using Sleep() 
rather than notifying waiters can't help your performance, either.  You 
should really use the standard locking primitives for this kind of 
thing.  (The thing you're trying to implement is called a reader-writer 
lock.)

> ERROR:src/openslide-decode-tiff.c:551:_openslide_tiffcache_destroy:
> assertion failed: (tc->outstanding == 0)

This means you're calling openslide_close() on a handle that's still 
being used in another thread.

The proper approach will depend on your use case.

- The reason closing and reopening the slide handle reduces memory use 
is that it frees the handle's tile cache.  If you are continually 
reading from each openslide_t (e.g. for batch processing of the entire 
slide), this can be counterproductive, because the tile cache is there 
to save you CPU time.  In this case, it'd be better to reduce your 
thread count.

- If you're only reading an occasional tile from each slide (e.g. your 
program is a tile server), it'd be better to keep a cache of 
recently-used openslide_t handles, rather than maintaining an 
openslide_t for each slide.  For example:

===
// Not tested or even compiled.  This started out as a "simple example"
// and then turned into a lot of code.  Sigh.

#define MAX_UNUSED_HANDLES 16

struct cache {
   GMutex *lock;
   GHashTable *entries;  // path -> cache_value *
   GQueue *freeable;
}

struct cache_value {
   openslide_t *osr;
   char *path;
   int users;
}

struct cache *cache_init(void) {
   struct cache *cache = g_slice_new0(struct cache);
   cache->lock = g_mutex_new();
   cache->entries = g_hash_table_new(g_str_hash, g_str_equal);
   cache->freeable = g_queue_new();
   return cache;
}

// cache lock must be held
static void _cache_free_one_entry(struct cache *cache) {
   struct cache_value *value = g_queue_pop_head(cache->freeable)
   g_assert(value && value->users == 0);
   g_hash_table_remove(cache->entries, value->path);
   g_free(value->path);
   openslide_close(value->osr);
   g_slice_free(struct cache_value, value);
}

void cache_destroy(struct cache *cache) {
   g_mutex_lock(cache->lock);
   struct cache_value *value;
   while (!g_queue_is_empty(cache->freeable)) {
     _cache_free_one_entry(cache);
   }
   // no cache entries should still be in use
   g_assert(g_hash_table_size(cache->entries) == 0);
   g_mutex_unlock(cache->lock);
   g_hash_table_destroy(cache->entries);
   g_queue_free(cache->freeable);
   g_mutex_free(cache->lock);
   g_slice_free(struct cache, cache);
}

openslide_t *cache_get(struct cache *cache, const char *path) {
   g_mutex_lock(cache->lock);
   struct cache_value *value = g_hash_table_lookup(cache->entries, path);
   if (!value) {
     value = g_slice_new0(struct cache_value);
     value->path = g_strdup(path);
     value->osr = openslide_open(path);
     if (!value->osr || openslide_get_error(value->osr)) {
        abort();  // don't actually do this
     }
     g_hash_table_insert(cache->entries, value->path, value);
   } else if (value->users == 0) {
     // We use the O(n) solution for clarity.  O(1) is possible by
     // tracking the list link in struct cache_value and using
     // g_queue_delete_link().
     g_queue_remove(cache->freeable, value);
   }
   value->users++;
   openslide_t *osr = value->osr;
   g_mutex_unlock(cache->lock);
   return osr;
}

void cache_put(struct cache *cache, const char *path, openslide_t *osr) {
   g_mutex_lock(cache->lock);
   struct cache_value *value = g_hash_table_lookup(cache->entries, path);
   g_assert(value && value->users && value->osr == osr);
   if (--value->users == 0) {
     g_queue_push_tail(cache->freeable, value);
     if (g_queue_get_length(cache->freeable) > MAX_UNUSED_HANDLES) {
       _cache_free_one_entry(cache);
     }
   }
   g_mutex_unlock(cache->lock);
}

void do_some_operation(struct cache *cache, const char *path) {
   openslide_t *osr = cache_get(cache, path);
   openslide_read_region(osr, ...);
   cache_put(cache, path, osr);
}
===

--Benjamin Gilbert