diff --git a/TODO b/TODO index 73d0c5e7..b7721578 100644 --- a/TODO +++ b/TODO @@ -1,3 +1,4 @@ + - rework buffer.c, it's getting ugly how about getting rid of buffercache for non-worker threads, it won't be @@ -8,6 +9,60 @@ no reserve list, just discard them on unref + shouldwe use atomic stuff for the buffer ref count? + + buffer_dump has: + + if( buffer->im && + buffer->buf && + buffer->cache ) { + printf( "buffer %p, %.3g MB\n", + buffer, buffer->bsize / (1024 * 1024.0) ); + *alive += buffer->bsize; + } + else if( buffer->im && + buffer->buf && + !buffer->cache ) + *reserve += buffer->bsize; + else + printf( "buffer craziness!\n" ); + + is this right? + + I think we now have three states: + + global buffer (not on any cache, undone) + + buffer->ref_count >= 1 + buffer->im + buffer->buf + !buffer->cache + !buffer->done (since all done buffers must be on a cache buffers list) + + buffer published on a cache (done) + + buffer->ref_count >= 1 + buffer->im + buffer->buf + buffer->cache + buffer->done + + buffer held in reserve on a thread, ready for reuse + + buffer->ref_count == 0 + buffer->im + buffer->buf + buffer->cache + !buffer->done + + vips_buffer_done() on a global buffer takes ownership and publishes to this + thread's public list + + what if ref_count > 1? is this possible? + + vips_buffer_undone() relinquishes ownership and makes it global + + - not sure about utf8 error messages on win - strange: diff --git a/libvips/include/vips/private.h b/libvips/include/vips/private.h index 84e46977..d9648190 100644 --- a/libvips/include/vips/private.h +++ b/libvips/include/vips/private.h @@ -89,13 +89,16 @@ typedef struct { GThread *thread; /* Just for sanity checking */ } VipsBufferThread; -/* Per-image buffer cache. Hash to this from VipsBufferThread::hash. +/* Per-image buffer cache. This keeps a list of "done" VipsBuffer that this + * worker has generated. We use this to reuse results within a thread. + * + * Hash to this from VipsBufferThread::hash. * We can't store the GSList directly in the hash table as GHashTable lacks an * update operation and we'd need to _remove() and _insert() on every list * operation. */ typedef struct _VipsBufferCache { - GSList *buffers; /* GSList of VipsBuffer* */ + GSList *buffers; /* GSList of "done" VipsBuffer* */ GThread *thread; /* Just for sanity checking */ struct _VipsImage *im; VipsBufferThread *buffer_thread; @@ -105,13 +108,15 @@ typedef struct _VipsBufferCache { /* What we track for each pixel buffer. These can move between caches and * between threads, but not between images. + * + * Moving between threads is difficult, use region ownership stuff. */ typedef struct _VipsBuffer { int ref_count; /* # of regions referencing us */ struct _VipsImage *im; /* VipsImage we are attached to */ VipsRect area; /* Area this pixel buffer covers */ - gboolean done; /* Calculated and in cache */ + gboolean done; /* Calculated and in a cache */ VipsBufferCache *cache; /* The cache this buffer is published on */ VipsPel *buf; /* Private malloc() area */ size_t bsize; /* Size of private malloc() */ diff --git a/libvips/iofuncs/buffer.c b/libvips/iofuncs/buffer.c index 23e450ad..fa801bd1 100644 --- a/libvips/iofuncs/buffer.c +++ b/libvips/iofuncs/buffer.c @@ -23,6 +23,8 @@ * 6/6/16 * - free buffers on image close as well as thread exit, so main thread * buffers don't clog up the system + * 13/10/16 + * - better solution: don't keep a buffercache for non-workers */ /* @@ -53,11 +55,10 @@ */ /* + */ #define DEBUG_VERBOSE #define DEBUG_CREATE #define DEBUG -#define DEBUG_GLOBAL - */ #ifdef HAVE_CONFIG_H #include @@ -85,21 +86,24 @@ static GSList *vips__buffer_cache_all = NULL; */ static const int buffer_cache_max_reserve = 2; -/* Workers have a buffer_thread in a GPrivate they have exclusive access to. +/* Workers have a BufferThread (and BufferCache) in a GPrivate they have + * exclusive access to. */ static GPrivate *buffer_thread_key = NULL; -/* All non-worker threads share a single global set of buffers protected by a - * mutex. - */ -static VipsBufferThread *vips_buffer_thread_global = NULL; - -#ifdef DEBUG_GLOBAL -/* Count main thread buffers in and out. - */ -static int vips_buffer_thread_global_n = 0; -static int vips_buffer_thread_global_highwater = 0; -#endif /*DEBUG_GLOBAL*/ +void +vips_buffer_print( VipsBuffer *buffer ) +{ + printf( "VipsBuffer: %p ref_count = %d, ", buffer, buffer->ref_count ); + printf( "im = %p, ", buffer->im ); + printf( "area.left = %d, ", buffer->area.left ); + printf( "area.top = %d, ", buffer->area.top ); + printf( "area.width = %d, ", buffer->area.width ); + printf( "area.height = %d, ", buffer->area.height ); + printf( "done = %d, ", buffer->done ); + printf( "buf = %p, ", buffer->buf ); + printf( "bsize = %zd\n", buffer->bsize ); +} #ifdef DEBUG static void * @@ -107,18 +111,38 @@ vips_buffer_dump( VipsBuffer *buffer, size_t *reserve, size_t *alive ) { vips_buffer_print( buffer ); - if( buffer->im && - buffer->buf && - buffer->cache ) { - printf( "buffer %p, %.3g MB\n", + g_assert( buffer->im ); + g_assert( buffer->buf ); + + if( !buffer->cache && + !buffer->done ) { + /* Global buffer, not linked to any cache. + */ + printf( "global buffer %p, %.3g MB\n", buffer, buffer->bsize / (1024 * 1024.0) ); *alive += buffer->bsize; } - else - if( buffer->im && - buffer->buf && - !buffer->cache ) + + else if( buffer->cache && + buffer->done && + !vips_rect_isempty( &buffer->area ) && + g_slist_find( buffer->cache->buffers, buffer ) ) { + /* Published on a thread. + */ + printf( "thread buffer %p, %.3g MB\n", + buffer, buffer->bsize / (1024 * 1024.0) ); + *alive += buffer->bsize; + } + + else if( buffer->ref_count == 0 && + buffer->cache && + !buffer->done && + vips_rect_isempty( &buffer->area ) && + g_slist_find( buffer->cache->reserve, buffer ) ) + /* Held in reserve. + */ *reserve += buffer->bsize; + else printf( "buffer craziness!\n" ); @@ -169,12 +193,6 @@ vips_buffer_dump_all( void ) } #endif /*DEBUG_CREATE*/ #endif /*DEBUG*/ - -#ifdef DEBUG_GLOBAL - printf( "buffers: %d global buffers\n", vips_buffer_thread_global_n ); - printf( "buffers: %d high water global buffers\n", - vips_buffer_thread_global_highwater ); -#endif /*DEBUG_GLOBAL*/ } static void @@ -197,20 +215,11 @@ vips_buffer_free( VipsBuffer *buffer ) static void buffer_thread_free( VipsBufferThread *buffer_thread ) { - /* We only come here from workers, so no need to lock. - */ VIPS_FREEF( g_hash_table_destroy, buffer_thread->hash ); VIPS_FREE( buffer_thread ); } -/* This can be called via two routes: - * - * - on thread shutdown, the enclosing hash is destroyed, and that will - * trigger this via GDestroyNotify. - * - if the BufferCache has been allocated by the main thread, this will be - * triggered from postclose on the image - * - * These can happen in either order. +/* Run for GDestroyNotify on the VipsBufferThread hash. */ static void buffer_cache_free( VipsBufferCache *cache ) @@ -235,7 +244,11 @@ buffer_cache_free( VipsBufferCache *cache ) for( p = cache->buffers; p; p = p->next ) { VipsBuffer *buffer = (VipsBuffer *) p->data; + g_assert( buffer->done ); + g_assert( buffer->cache == cache ); + buffer->done = FALSE; + buffer->cache = NULL; } VIPS_FREEF( g_slist_free, cache->buffers ); @@ -249,32 +262,6 @@ buffer_cache_free( VipsBufferCache *cache ) g_free( cache ); } -static void -buffer_cache_image_postclose( VipsImage *im, VipsBufferCache *cache ) -{ - VipsBufferThread *buffer_thread = cache->buffer_thread; - - /* Runs to clean up main thread buffers on image close. - */ - g_assert( cache->im == im ); - g_assert( !vips_thread_isworker() ); - - /* All non-worker threads come through here, so we need to lock around - * changes to the global buffer_thread. - */ - g_mutex_lock( vips__global_lock ); - - g_hash_table_remove( buffer_thread->hash, im ); - -#ifdef DEBUG_GLOBAL - vips_buffer_thread_global_n -= 1; - printf( "buffer_cache_image_postclose: %d global buffers\n", - vips_buffer_thread_global_n ); -#endif /*DEBUG_GLOBAL*/ - - g_mutex_unlock( vips__global_lock ); -} - static VipsBufferCache * buffer_cache_new( VipsBufferThread *buffer_thread, VipsImage *im ) { @@ -288,29 +275,6 @@ buffer_cache_new( VipsBufferThread *buffer_thread, VipsImage *im ) cache->reserve = NULL; cache->n_reserve = 0; - /* VipsBufferCache allocated from worker threads will be freed when - * workers shut down. This won't happen for VipsBufferCache allocated - * from the main thread, since (obviously) thread shutdown will never - * happen. In this case, we need to free resources on image close. - */ - if( !vips_thread_isworker() ) { - g_signal_connect( im, "postclose", - G_CALLBACK( buffer_cache_image_postclose ), cache ); - -#ifdef DEBUG_GLOBAL - /* No need to lock. Main thread buffer_cache_new() calls are - * always inside a lock already. - */ - vips_buffer_thread_global_n += 1; - vips_buffer_thread_global_highwater = VIPS_MAX( - vips_buffer_thread_global_highwater, - vips_buffer_thread_global_n ); - - printf( "buffer_cache_new: %d global buffers\n", - vips_buffer_thread_global_n ); -#endif /*DEBUG_GLOBAL*/ - } - #ifdef DEBUG_CREATE g_mutex_lock( vips__global_lock ); vips__buffer_cache_all = @@ -340,6 +304,8 @@ buffer_thread_new( void ) return( buffer_thread ); } +/* Get our private VipsBufferThread. NULL for non-worker threads. + */ static VipsBufferThread * buffer_thread_get( void ) { @@ -355,47 +321,33 @@ buffer_thread_get( void ) g_assert( buffer_thread->thread == g_thread_self() ); } - else { - /* All main threads share a single set of buffers. + else + /* Non-workers don't have one. */ - g_mutex_lock( vips__global_lock ); - - if( !vips_buffer_thread_global ) { - vips_buffer_thread_global = buffer_thread_new(); - - /* Shared by many threads, so no checking. - */ - vips_buffer_thread_global->thread = NULL; - } - buffer_thread = vips_buffer_thread_global; - - g_mutex_unlock( vips__global_lock ); - } + buffer_thread = NULL; return( buffer_thread ); } +/* Get the VipsBufferCache for this image, or NULL for a non-worker. + */ static VipsBufferCache * buffer_cache_get( VipsImage *im ) { - VipsBufferThread *buffer_thread = buffer_thread_get(); - + VipsBufferThread *buffer_thread; VipsBufferCache *cache; - if( !vips_thread_isworker() ) - g_mutex_lock( vips__global_lock ); + if( (buffer_thread = buffer_thread_get()) ) { + if( !(cache = (VipsBufferCache *) + g_hash_table_lookup( buffer_thread->hash, im )) ) { + cache = buffer_cache_new( buffer_thread, im ); + g_hash_table_insert( buffer_thread->hash, im, cache ); + } - if( !(cache = (VipsBufferCache *) - g_hash_table_lookup( buffer_thread->hash, im )) ) { - cache = buffer_cache_new( buffer_thread, im ); - g_hash_table_insert( buffer_thread->hash, im, cache ); + g_assert( cache->thread == g_thread_self() ); } - - if( !vips_thread_isworker() ) - g_mutex_unlock( vips__global_lock ); - - g_assert( !cache->thread || - cache->thread == g_thread_self() ); + else + cache = NULL; return( cache ); } @@ -405,10 +357,11 @@ buffer_cache_get( VipsImage *im ) void vips_buffer_done( VipsBuffer *buffer ) { - if( !buffer->done ) { - VipsImage *im = buffer->im; - VipsBufferCache *cache = buffer_cache_get( im ); + VipsImage *im = buffer->im; + VipsBufferCache *cache; + if( !buffer->done && + (cache = buffer_cache_get( im )) ) { #ifdef DEBUG_VERBOSE printf( "vips_buffer_done: thread %p adding to cache %p\n", g_thread_self(), cache ); @@ -439,20 +392,18 @@ vips_buffer_undone( VipsBuffer *buffer ) g_thread_self(), buffer, cache ); #endif /*DEBUG_VERBOSE*/ - g_assert( !cache->thread || - cache->thread == g_thread_self() ); - g_assert( !cache->thread || - cache->buffer_thread->thread == cache->thread ); + g_assert( cache->thread == g_thread_self() ); + g_assert( cache->buffer_thread->thread == cache->thread ); g_assert( g_slist_find( cache->buffers, buffer ) ); + g_assert( buffer_thread_get() ); g_assert( cache->buffer_thread == buffer_thread_get() ); cache->buffers = g_slist_remove( cache->buffers, buffer ); - buffer->done = FALSE; #ifdef DEBUG_VERBOSE printf( "vips_buffer_undone: %d buffers left\n", - g_slist_length( cache_list->buffers ) ); + g_slist_length( cache->buffers ) ); #endif /*DEBUG_VERBOSE*/ } @@ -477,13 +428,7 @@ vips_buffer_unref( VipsBuffer *buffer ) buffer->ref_count -= 1; if( buffer->ref_count == 0 ) { - /* We are not always the creating thread, for example if we - * come here during vips_region_dispose(). cache may have been - * NULLed out during thread exit. - VipsBufferCache *cache = buffer->cache; - */ - - VipsBufferCache *cache = buffer_cache_get( buffer->im ); + VipsBufferCache *cache; #ifdef DEBUG_VERBOSE if( !buffer->done ) @@ -494,7 +439,7 @@ vips_buffer_unref( VipsBuffer *buffer ) /* Place on this thread's reserve list for reuse. */ - if( cache && + if( (cache = buffer_cache_get( buffer->im )) && cache->n_reserve < buffer_cache_max_reserve ) { g_assert( !buffer->cache ); @@ -502,6 +447,7 @@ vips_buffer_unref( VipsBuffer *buffer ) g_slist_prepend( cache->reserve, buffer ); cache->n_reserve += 1; + buffer->cache = cache; buffer->area.width = 0; buffer->area.height = 0; } @@ -541,20 +487,21 @@ buffer_move( VipsBuffer *buffer, VipsRect *area ) VipsBuffer * vips_buffer_new( VipsImage *im, VipsRect *area ) { - VipsBufferCache *cache = buffer_cache_get( im ); - + VipsBufferCache *cache; VipsBuffer *buffer; - if( cache->reserve ) { + if( (cache = buffer_cache_get( im )) && + cache->reserve ) { buffer = (VipsBuffer *) cache->reserve->data; cache->reserve = g_slist_remove( cache->reserve, buffer ); cache->n_reserve -= 1; g_assert( buffer->im == im ); g_assert( buffer->done == FALSE ); - g_assert( !buffer->cache ); + g_assert( buffer->cache ); buffer->ref_count = 1; + buffer->cache = NULL; } else { buffer = g_new0( VipsBuffer, 1 ); @@ -581,19 +528,23 @@ vips_buffer_new( VipsImage *im, VipsRect *area ) return( buffer ); } -/* Find an existing buffer that encloses area and return a ref. +/* Find an existing buffer that encloses area and return a ref. Or NULL for no + * existing buffer. */ static VipsBuffer * buffer_find( VipsImage *im, VipsRect *r ) { - VipsBufferCache *cache = buffer_cache_get( im ); - + VipsBufferCache *cache; VipsBuffer *buffer; GSList *p; VipsRect *area; + if( !(cache = buffer_cache_get( im )) ) + return( NULL ); + /* This needs to be quick :-( don't use - * vips_slist_map2()/vips_rect_includesrect(), do the search inline. + * vips_slist_map2()/vips_rect_includesrect(), do the search + * inline. * * FIXME we return the first enclosing buffer, perhaps we should * search for the largest? @@ -609,7 +560,7 @@ buffer_find( VipsImage *im, VipsRect *r ) buffer->ref_count += 1; #ifdef DEBUG_VERBOSE - printf( "vips_buffer_find: left = %d, top = %d, " + printf( "buffer_find: left = %d, top = %d, " "width = %d, height = %d, count = %d (%p)\n", buffer->area.left, buffer->area.top, buffer->area.width, buffer->area.height, @@ -632,13 +583,10 @@ vips_buffer_ref( VipsImage *im, VipsRect *area ) { VipsBuffer *buffer; - if( !(buffer = buffer_find( im, area )) ) - /* No existing buffer ... make a new one. - */ - if( !(buffer = vips_buffer_new( im, area )) ) - return( NULL ); - - return( buffer ); + if( (buffer = buffer_find( im, area )) ) + return( buffer ); + else + return( vips_buffer_new( im, area ) ); } /* Unref old, ref new, in a single operation. Reuse stuff if we can. The @@ -686,20 +634,6 @@ vips_buffer_unref_ref( VipsBuffer *old_buffer, VipsImage *im, VipsRect *area ) return( buffer ); } -void -vips_buffer_print( VipsBuffer *buffer ) -{ - printf( "VipsBuffer: %p ref_count = %d, ", buffer, buffer->ref_count ); - printf( "im = %p, ", buffer->im ); - printf( "area.left = %d, ", buffer->area.left ); - printf( "area.top = %d, ", buffer->area.top ); - printf( "area.width = %d, ", buffer->area.width ); - printf( "area.height = %d, ", buffer->area.height ); - printf( "done = %d, ", buffer->done ); - printf( "buf = %p, ", buffer->buf ); - printf( "bsize = %zd\n", buffer->bsize ); -} - static void buffer_thread_destroy_notify( VipsBufferThread *buffer_thread ) {