From 9cb152596c3cc5b83abcf5f07dd2ef41509ff381 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Wed, 29 Jan 2014 09:07:58 +0000 Subject: [PATCH] bg render thread quits on shutdown fixes a small memleak --- ChangeLog | 1 + TODO | 36 -------------- libvips/include/vips/private.h | 6 ++- libvips/iofuncs/buffer.c | 89 +++++++++++++++++++++++++--------- libvips/iofuncs/init.c | 6 +++ libvips/iofuncs/sinkscreen.c | 80 +++++++++++++++++++----------- 6 files changed, 129 insertions(+), 89 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9fe97e87..ec45da72 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,6 @@ 21/1/14 started 7.39.0 - auto-decode for (almost) all operations +- background render thread cleans up and quits neatly 22/1/14 started 7.38.2 - auto RAD decode for affine diff --git a/TODO b/TODO index f4411419..469c8703 100644 --- a/TODO +++ b/TODO @@ -1,39 +1,3 @@ -- leak ... nip2 ~/pics/babe.jpg, ^Q - -==4389== 784 bytes in 1 blocks are possibly lost in loss record 14,881 of -15,393 -==4389== at 0x4C2A2DB: malloc (in -/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) -==4389== by 0x6749221: vips_tracked_malloc (memory.c:289) -==4389== by 0x67587E9: buffer_move (buffer.c:368) -==4389== by 0x675898B: vips_buffer_new (buffer.c:412) -==4389== by 0x6758C11: vips_buffer_unref_ref (buffer.c:519) -==4389== by 0x674F5FB: vips_region_buffer (region.c:602) -==4389== by 0x674FF73: vips_region_fill (region.c:887) -==4389== by 0x67505AD: vips_region_prepare (region.c:1139) -==4389== by 0x67400FB: vips_image_write_gen (image.c:1930) -==4389== by 0x67504CF: vips_region_generate (region.c:1074) -==4389== by 0x6750718: vips_region_prepare_to_generate (region.c:1196) -==4389== by 0x6750A55: vips_region_prepare_to (region.c:1315) - -==4389== 784 bytes in 1 blocks are possibly lost in loss record 14,882 of -15,393 -==4389== at 0x4C2A2DB: malloc (in -/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) -==4389== by 0x6749221: vips_tracked_malloc (memory.c:289) -==4389== by 0x67587E9: buffer_move (buffer.c:368) -==4389== by 0x6758BC6: vips_buffer_unref_ref (buffer.c:508) -==4389== by 0x674F5FB: vips_region_buffer (region.c:602) -==4389== by 0x674FF73: vips_region_fill (region.c:887) -==4389== by 0x67505AD: vips_region_prepare (region.c:1139) -==4389== by 0x67400FB: vips_image_write_gen (image.c:1930) -==4389== by 0x67504CF: vips_region_generate (region.c:1074) -==4389== by 0x6750718: vips_region_prepare_to_generate (region.c:1196) -==4389== by 0x6750A55: vips_region_prepare_to (region.c:1315) -==4389== by 0x6747EC1: render_work (sinkscreen.c:418) - - - - vips_colourspace() needs an option from_space param,? diff --git a/libvips/include/vips/private.h b/libvips/include/vips/private.h index 6d3a1f89..a1ee8ebc 100644 --- a/libvips/include/vips/private.h +++ b/libvips/include/vips/private.h @@ -89,8 +89,8 @@ typedef struct { GThread *thread; /* Just for sanity checking */ } VipsBufferThread; -/* Per-image buffer cache. Hash to this from VipsBufferCache. - * We can't store the GSList directly in the hash table, as GHashTable lacks an +/* Per-image buffer cache. 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. */ @@ -127,6 +127,8 @@ VipsBuffer *vips_buffer_unref_ref( VipsBuffer *buffer, struct _VipsImage *im, VipsRect *area ); void vips_buffer_print( VipsBuffer *buffer ); +void vips__render_shutdown( void ); + /* Sections of region.h that are private to VIPS. */ diff --git a/libvips/iofuncs/buffer.c b/libvips/iofuncs/buffer.c index 62299a3f..950a16ef 100644 --- a/libvips/iofuncs/buffer.c +++ b/libvips/iofuncs/buffer.c @@ -50,10 +50,10 @@ */ /* -#define DEBUG_CREATE #define DEBUG_VERBOSE -#define DEBUG +#define DEBUG_CREATE */ +#define DEBUG #ifdef HAVE_CONFIG_H #include @@ -70,11 +70,11 @@ #ifdef DEBUG /* Track all regions here for debugging. */ -static GSList *vips__buffers_all = NULL; +static GSList *vips__buffer_all = NULL; #endif /*DEBUG*/ #ifdef DEBUG_CREATE -static int buffer_cache_n = 0; +static GSList *vips__buffer_cache_all = NULL; #endif /*DEBUG_CREATE*/ /* The maximum numbers of buffers we hold in reserve per image. @@ -87,13 +87,19 @@ static GPrivate *buffer_thread_key = NULL; static void * vips_buffer_dump( VipsBuffer *buffer, size_t *reserve, size_t *alive ) { + vips_buffer_print( buffer ); + if( buffer->im && - buffer->buf ) { - printf( "buffer %p, %gMB\n", + buffer->buf && + buffer->cache ) { + printf( "buffer %p, %.3g MB\n", buffer, buffer->bsize / (1024 * 1024.0) ); *alive += buffer->bsize; } - else if( !buffer->im ) + else + if( buffer->im && + buffer->buf && + !buffer->cache ) *reserve += buffer->bsize; else printf( "buffer craziness!\n" ); @@ -101,18 +107,47 @@ vips_buffer_dump( VipsBuffer *buffer, size_t *reserve, size_t *alive ) return( NULL ); } +#ifdef DEBUG_CREATE +static void * +vips_buffer_cache_dump( VipsBufferCache *cache ) +{ + printf( "VipsBufferCache: %p\n", cache ); + printf( "\t%d buffers\n", g_slist_length( cache->buffers ) ); + printf( "\tthread %p\n", cache->thread ); + printf( "\timage %p\n", cache->im ); + printf( "\tbuffer_thread %p\n", cache->buffer_thread ); + printf( "\t%d in reserve\n", g_slist_length( cache->reserve ) ); + + return( NULL ); +} +#endif /*DEBUG_CREATE*/ + void vips_buffer_dump_all( void ) { - size_t reserve; - size_t alive; + if( vips__buffer_all ) { + size_t reserve; + size_t alive; - reserve = 0; - alive = 0; - vips_slist_map2( vips__buffers_all, - (VipsSListMap2Fn) vips_buffer_dump, &reserve, &alive ); - printf( "%gMB alive\n", alive / (1024 * 1024.0) ); - printf( "%gMB in reserve\n", reserve / (1024 * 1024.0) ); + printf( "buffers:\n" ); + + reserve = 0; + alive = 0; + vips_slist_map2( vips__buffer_all, + (VipsSListMap2Fn) vips_buffer_dump, &reserve, &alive ); + printf( "%.3g MB alive\n", alive / (1024 * 1024.0) ); + printf( "%.3g MB in reserve\n", reserve / (1024 * 1024.0) ); + } + +#ifdef DEBUG_CREATE + if( vips__buffer_cache_all ) { + printf( "buffers: %d buffer cache still alive\n", + g_slist_length( vips__buffer_cache_all ) ); + vips_slist_map2( vips__buffer_cache_all, + (VipsSListMap2Fn) vips_buffer_cache_dump, NULL, NULL ); + printf( "g_thread_self() == %p\n", g_thread_self() ); + } +#endif /*DEBUG_CREATE*/ } #endif /*DEBUG*/ @@ -126,8 +161,8 @@ vips_buffer_free( VipsBuffer *buffer ) #ifdef DEBUG g_mutex_lock( vips__global_lock ); - g_assert( g_slist_find( vips__buffers_all, buffer ) ); - vips__buffers_all = g_slist_remove( vips__buffers_all, buffer ); + g_assert( g_slist_find( vips__buffer_all, buffer ) ); + vips__buffer_all = g_slist_remove( vips__buffer_all, buffer ); g_mutex_unlock( vips__global_lock ); #endif /*DEBUG*/ @@ -146,11 +181,15 @@ buffer_cache_free( VipsBufferCache *cache ) GSList *p; #ifdef DEBUG_CREATE - buffer_cache_n -= 1; + g_mutex_lock( vips__global_lock ); + vips__buffer_cache_all = + g_slist_remove( vips__buffer_cache_all, cache ); + g_mutex_unlock( vips__global_lock ); printf( "buffer_cache_free: freeing cache %p on thread %p\n", cache, g_thread_self() ); - printf( "\t(%d caches left)\n", buffer_cache_n ); + printf( "\t(%d caches left)\n", + g_slist_length( vips__buffer_cache_all ) ); #endif /*DEBUG_CREATE*/ /* Need to mark undone so we don't try and take them off this hash on @@ -187,11 +226,15 @@ buffer_cache_new( VipsBufferThread *buffer_thread, VipsImage *im ) cache->n_reserve = 0; #ifdef DEBUG_CREATE - buffer_cache_n += 1; + g_mutex_lock( vips__global_lock ); + vips__buffer_cache_all = + g_slist_prepend( vips__buffer_cache_all, cache ); + g_mutex_unlock( vips__global_lock ); printf( "buffer_cache_new: new cache %p for thread %p\n", cache, g_thread_self() ); - printf( "\t(%d caches now)\n", buffer_cache_n ); + printf( "\t(%d caches now)\n", + g_slist_length( vips__buffer_cache_all ) ); #endif /*DEBUG_CREATE*/ return( cache ); @@ -403,8 +446,8 @@ vips_buffer_new( VipsImage *im, VipsRect *area ) #ifdef DEBUG g_mutex_lock( vips__global_lock ); - vips__buffers_all = - g_slist_prepend( vips__buffers_all, buffer ); + vips__buffer_all = + g_slist_prepend( vips__buffer_all, buffer ); g_mutex_unlock( vips__global_lock ); #endif /*DEBUG*/ } diff --git a/libvips/iofuncs/init.c b/libvips/iofuncs/init.c index 7ab48b77..cda33dbb 100644 --- a/libvips/iofuncs/init.c +++ b/libvips/iofuncs/init.c @@ -368,6 +368,10 @@ vips_leak( void ) fprintf( stderr, "%s", vips_buf_all( &buf ) ); vips__type_leak(); + +#ifdef DEBUG +#endif /*DEBUG*/ + vips_buffer_dump_all(); } /** @@ -418,6 +422,8 @@ vips_shutdown( void ) vips__thread_gate_stop( "init: main" ); } + vips__render_shutdown(); + vips_thread_shutdown(); vips__thread_profile_stop(); diff --git a/libvips/iofuncs/sinkscreen.c b/libvips/iofuncs/sinkscreen.c index 7ecbdc65..71747254 100644 --- a/libvips/iofuncs/sinkscreen.c +++ b/libvips/iofuncs/sinkscreen.c @@ -5,6 +5,8 @@ * 25/11/10 * - in synchronous mode, use a single region for input and save huge * mem use + * 20/1/14 + * - bg render thread quits on shutdown */ /* @@ -160,6 +162,10 @@ static GThread *render_thread = NULL; */ static VipsSemaphore render_dirty_sem; +/* Set this to ask the render thread to quit. + */ +static gboolean render_kill = FALSE; + /* All the renders with dirty tiles. */ static GMutex *render_dirty_lock = NULL; @@ -476,44 +482,62 @@ render_dirty_put( Render *render ) static void * render_thread_main( void *client ) { - for(;;) { - Render *render; + Render *render; - if( (render = render_dirty_get()) ) { - /* Ignore errors, I'm not sure what we'd do with them - * anyway. - */ - VIPS_DEBUG_MSG_GREEN( "render_thread_main: " - "threadpool start\n" ); + while( (render = render_dirty_get()) && + !render_kill ) { + VIPS_DEBUG_MSG_GREEN( "render_thread_main: " + "threadpool start\n" ); - render_reschedule = FALSE; - if( vips_threadpool_run( render->in, - render_thread_state_new, - render_allocate, - render_work, - NULL, - render ) ) - VIPS_DEBUG_MSG_RED( "render_thread_main: " - "threadpool_run failed\n" ); + render_reschedule = FALSE; + if( vips_threadpool_run( render->in, + render_thread_state_new, + render_allocate, + render_work, + NULL, + render ) ) + VIPS_DEBUG_MSG_RED( "render_thread_main: " + "threadpool_run failed\n" ); - VIPS_DEBUG_MSG_GREEN( "render_thread_main: " - "threadpool return\n" ); + VIPS_DEBUG_MSG_GREEN( "render_thread_main: " + "threadpool return\n" ); - /* Add back to the jobs list, if we need to. - */ - render_dirty_put( render ); + /* Add back to the jobs list, if we need to. + */ + render_dirty_put( render ); - /* _get() does a ref to make sure we keep the render - * alive during processing ... unref before we loop. - * This can kill off the render. - */ - render_unref( render ); - } + /* _get() does a ref to make sure we keep the render + * alive during processing ... unref before we loop. + * This can kill off the render. + */ + render_unref( render ); } return( NULL ); } +void +vips__render_shutdown( void ) +{ + g_mutex_lock( render_dirty_lock ); + + if( render_thread ) { + GThread *thread; + + thread = render_thread; + render_thread = NULL; + + g_mutex_unlock( render_dirty_lock ); + + render_reschedule = TRUE; + render_kill = TRUE; + vips_semaphore_up( &render_dirty_sem ); + (void) g_thread_join( thread ); + } + else + g_mutex_unlock( render_dirty_lock ); +} + /* Create our set of RenderThread. Assume we're single-threaded here. */ static int