From 2c5ee332f02fd92b5ddad0e423dd6102c7e95872 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Wed, 18 Dec 2013 09:54:26 +0000 Subject: [PATCH] make the buffer recycle list per image so now recycle lists are short, scale with pipeline complexity, and buffers are always appropriately sized for the image instead of being slowly sized up to the max size for the pipeline before: $ vips sharpen k2.jpg x.jpg --radius 20 memory: high-water mark 38.99 MB after: $ vips sharpen k2.jpg x.jpg --radius 20 memory: high-water mark 29.46 MB --- TODO | 3 + libvips/include/vips/private.h | 21 ++-- libvips/iofuncs/buffer.c | 210 ++++++++++++++++----------------- tools/vipsprofile | 9 ++ 4 files changed, 125 insertions(+), 118 deletions(-) diff --git a/TODO b/TODO index f8ea7a4a..6502c5cf 100644 --- a/TODO +++ b/TODO @@ -1,3 +1,6 @@ +- vipsthumbnail could have a --zoom option, does a crop to ensure output is + always exactly the specified --size + - vipsprofile reports a leak, strangely - vipsprofile performance is very poor for large data sets, eg. diff --git a/libvips/include/vips/private.h b/libvips/include/vips/private.h index b8ee9522..6d3a1f89 100644 --- a/libvips/include/vips/private.h +++ b/libvips/include/vips/private.h @@ -82,28 +82,29 @@ VipsWindow *vips_window_ref( struct _VipsImage *im, int top, int height ); int vips_window_unref( VipsWindow *window ); void vips_window_print( VipsWindow *window ); -/* Per-thread buffer cache. Held in a GPrivate. +/* Per-thread buffer state. Held in a GPrivate. */ typedef struct { - GHashTable *hash; /* Hash to VipsBufferCacheList* */ + GHashTable *hash; /* VipsImage -> VipsBufferCache* */ GThread *thread; /* Just for sanity checking */ - GSList *reserve; /* VipsBuffer kept in reserve */ - int n_reserve; /* Number in reserve */ -} VipsBufferCache; +} 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 * update operation and we'd need to _remove() and _insert() on every list * operation. */ -typedef struct _VipsBufferCacheList { +typedef struct _VipsBufferCache { GSList *buffers; /* GSList of VipsBuffer* */ GThread *thread; /* Just for sanity checking */ struct _VipsImage *im; - VipsBufferCache *cache; -} VipsBufferCacheList; + VipsBufferThread *buffer_thread; + GSList *reserve; /* VipsBuffer kept in reserve */ + int n_reserve; /* Number in reserve */ +} VipsBufferCache; -/* What we track for each pixel buffer. +/* What we track for each pixel buffer. These can move between caches and + * between threads, but not between images. */ typedef struct _VipsBuffer { int ref_count; /* # of regions referencing us */ @@ -111,7 +112,7 @@ typedef struct _VipsBuffer { VipsRect area; /* Area this pixel buffer covers */ gboolean done; /* Calculated and in cache */ - VipsBufferCache *cache; + VipsBufferCache *cache; /* The cache this buffer is published on */ VipsPel *buf; /* Private malloc() area */ size_t bsize; /* Size of private malloc() */ } VipsBuffer; diff --git a/libvips/iofuncs/buffer.c b/libvips/iofuncs/buffer.c index 8521802a..7e2a9863 100644 --- a/libvips/iofuncs/buffer.c +++ b/libvips/iofuncs/buffer.c @@ -17,6 +17,9 @@ * - move link maintenance to im_demand_hint * 21/9/11 * - switch to vips_tracked_malloc() + * 18/12/13 + * - keep a few buffers in reserve per image, stops malloc/free + * cycling when sharing is repeatedly discovered */ /* @@ -74,12 +77,11 @@ static GSList *vips__buffers_all = NULL; static int buffer_cache_n = 0; #endif /*DEBUG_CREATE*/ -/* The maximum numbers of buffers we hold in reserve per thread. About 5 seems - * enough to stop malloc cycling on vips_sharpen(). +/* The maximum numbers of buffers we hold in reserve per image. */ -static const int buffer_cache_max_reserve = 40; +static const int buffer_cache_max_reserve = 2; -static GPrivate *thread_buffer_cache_key = NULL; +static GPrivate *buffer_thread_key = NULL; #ifdef DEBUG static void * @@ -131,6 +133,13 @@ vips_buffer_free( VipsBuffer *buffer ) #endif /*DEBUG*/ } +static void +buffer_thread_free( VipsBufferThread *buffer_thread ) +{ + VIPS_FREEF( g_hash_table_destroy, buffer_thread->hash ); + VIPS_FREE( buffer_thread ); +} + static void buffer_cache_free( VipsBufferCache *cache ) { @@ -139,69 +148,41 @@ buffer_cache_free( VipsBufferCache *cache ) #ifdef DEBUG_CREATE buffer_cache_n -= 1; - printf( "vips__buffer_cache_free: freeing cache %p on thread %p\n", + printf( "buffer_cache_free: freeing cache %p on thread %p\n", cache, g_thread_self() ); printf( "\t(%d caches left)\n", buffer_cache_n ); #endif /*DEBUG_CREATE*/ + /* Need to mark undone so we don't try and take them off this hash on + * unref. + */ + for( p = cache->buffers; p; p = p->next ) { + VipsBuffer *buffer = (VipsBuffer *) p->data; + + buffer->done = FALSE; + } + VIPS_FREEF( g_slist_free, cache->buffers ); + for( p = cache->reserve; p; p = p->next ) { VipsBuffer *buffer = (VipsBuffer *) p->data; vips_buffer_free( buffer ); } - VIPS_FREEF( g_slist_free, cache->reserve ); - VIPS_FREEF( g_hash_table_destroy, cache->hash ); - VIPS_FREE( cache ); -} -static void -buffer_cache_list_free( VipsBufferCacheList *cache_list ) -{ - GSList *p; - - /* Need to mark undone so we don't try and take them off this hash on - * unref. - */ - for( p = cache_list->buffers; p; p = p->next ) { - VipsBuffer *buffer = (VipsBuffer *) p->data; - - buffer->done = FALSE; - } - - VIPS_FREEF( g_slist_free, cache_list->buffers ); - g_free( cache_list ); -} - -static VipsBufferCacheList * -buffer_cache_list_new( VipsBufferCache *cache, VipsImage *im ) -{ - VipsBufferCacheList *cache_list; - - cache_list = g_new( VipsBufferCacheList, 1 ); - cache_list->buffers = NULL; - cache_list->thread = g_thread_self(); - cache_list->cache = cache; - cache_list->im = im; - -#ifdef DEBUG_CREATE - printf( "buffer_cache_list_new: new cache %p for thread %p\n", - cache, g_thread_self() ); - printf( "\t(%d caches now)\n", buffer_cache_n ); -#endif /*DEBUG_CREATE*/ - - return( cache_list ); + g_free( cache ); } static VipsBufferCache * -buffer_cache_new( void ) +buffer_cache_new( VipsBufferThread *buffer_thread, VipsImage *im ) { VipsBufferCache *cache; cache = g_new( VipsBufferCache, 1 ); - cache->hash = g_hash_table_new_full( g_direct_hash, g_direct_equal, - NULL, (GDestroyNotify) buffer_cache_list_free ); + cache->buffers = NULL; cache->thread = g_thread_self(); + cache->im = im; + cache->buffer_thread = buffer_thread; cache->reserve = NULL; cache->n_reserve = 0; @@ -216,19 +197,51 @@ buffer_cache_new( void ) return( cache ); } -/* Get the buffer cache. - */ -static VipsBufferCache * -buffer_cache_get( void ) +static VipsBufferThread * +buffer_thread_new( void ) { - VipsBufferCache *cache; + VipsBufferThread *buffer_thread; - if( !(cache = g_private_get( thread_buffer_cache_key )) ) { - cache = buffer_cache_new(); - g_private_set( thread_buffer_cache_key, cache ); + buffer_thread = g_new( VipsBufferThread, 1 ); + buffer_thread->hash = g_hash_table_new_full( + g_direct_hash, g_direct_equal, + NULL, (GDestroyNotify) buffer_cache_free ); + buffer_thread->thread = g_thread_self(); + + return( buffer_thread ); +} + +static VipsBufferThread * +buffer_thread_get( void ) +{ + VipsBufferThread *buffer_thread; + + if( !(buffer_thread = g_private_get( buffer_thread_key )) ) { + buffer_thread = buffer_thread_new(); + g_private_set( buffer_thread_key, buffer_thread ); } - return( cache ); + g_assert( buffer_thread->thread == g_thread_self() ); + + return( buffer_thread ); +} + +static VipsBufferCache * +buffer_cache_get( VipsImage *im ) +{ + VipsBufferThread *buffer_thread = buffer_thread_get(); + + VipsBufferCache *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() ); + + return( cache ); } /* Pixels have been calculated: publish for other parts of this thread to see. @@ -238,8 +251,7 @@ vips_buffer_done( VipsBuffer *buffer ) { if( !buffer->done ) { VipsImage *im = buffer->im; - VipsBufferCache *cache = buffer_cache_get(); - VipsBufferCacheList *cache_list; + VipsBufferCache *cache = buffer_cache_get( im ); #ifdef DEBUG_VERBOSE printf( "vips_buffer_done: thread %p adding to cache %p\n", @@ -247,18 +259,11 @@ vips_buffer_done( VipsBuffer *buffer ) vips_buffer_print( buffer ); #endif /*DEBUG_VERBOSE*/ - /* Look up and update the buffer list. - */ - if( !(cache_list = g_hash_table_lookup( cache->hash, im )) ) { - cache_list = buffer_cache_list_new( cache, im ); - g_hash_table_insert( cache->hash, im, cache_list ); - } + g_assert( !g_slist_find( cache->buffers, buffer ) ); + g_assert( !buffer->cache ); - g_assert( !g_slist_find( cache_list->buffers, buffer ) ); - g_assert( cache_list->thread == cache->thread ); + cache->buffers = g_slist_prepend( cache->buffers, buffer ); - cache_list->buffers = - g_slist_prepend( cache_list->buffers, buffer ); buffer->done = TRUE; buffer->cache = cache; } @@ -270,9 +275,8 @@ void vips_buffer_undone( VipsBuffer *buffer ) { if( buffer->done ) { - VipsImage *im = buffer->im; VipsBufferCache *cache = buffer->cache; - VipsBufferCacheList *cache_list; + VipsBufferThread *buffer_thread = cache->buffer_thread; #ifdef DEBUG_VERBOSE printf( "vips_buffer_undone: thread %p removing " @@ -281,19 +285,15 @@ vips_buffer_undone( VipsBuffer *buffer ) #endif /*DEBUG_VERBOSE*/ g_assert( cache->thread == g_thread_self() ); + g_assert( buffer_thread->thread == cache->thread ); + g_assert( g_slist_find( cache->buffers, buffer ) ); + g_assert( buffer_thread == buffer_thread_get() ); - cache_list = g_hash_table_lookup( cache->hash, im ); + cache->buffers = g_slist_remove( cache->buffers, buffer ); - g_assert( cache_list ); - g_assert( cache_list->thread == cache->thread ); - g_assert( g_slist_find( cache_list->buffers, buffer ) ); - - cache_list->buffers = - g_slist_remove( cache_list->buffers, buffer ); buffer->done = FALSE; buffer->cache = NULL; - #ifdef DEBUG_VERBOSE printf( "vips_buffer_undone: %d buffers left\n", g_slist_length( cache_list->buffers ) ); @@ -320,7 +320,8 @@ vips_buffer_unref( VipsBuffer *buffer ) buffer->ref_count -= 1; if( buffer->ref_count == 0 ) { - VipsBufferCache *cache = buffer_cache_get(); + VipsImage *im = buffer->im; + VipsBufferCache *cache = buffer_cache_get( im ); #ifdef DEBUG_VERBOSE if( !buffer->done ) @@ -336,9 +337,6 @@ vips_buffer_unref( VipsBuffer *buffer ) g_slist_prepend( cache->reserve, buffer ); cache->n_reserve += 1; - buffer->done = FALSE; - buffer->cache = NULL; - buffer->im = NULL; buffer->area.width = 0; buffer->area.height = 0; } @@ -378,7 +376,7 @@ buffer_move( VipsBuffer *buffer, VipsRect *area ) VipsBuffer * vips_buffer_new( VipsImage *im, VipsRect *area ) { - VipsBufferCache *cache = buffer_cache_get(); + VipsBufferCache *cache = buffer_cache_get( im ); VipsBuffer *buffer; @@ -387,10 +385,11 @@ vips_buffer_new( VipsImage *im, VipsRect *area ) 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 ); + buffer->ref_count = 1; - buffer->im = im; - buffer->done = FALSE; - buffer->cache = NULL; } else { buffer = g_new0( VipsBuffer, 1 ); @@ -422,22 +421,19 @@ vips_buffer_new( VipsImage *im, VipsRect *area ) static VipsBuffer * buffer_find( VipsImage *im, VipsRect *r ) { - VipsBufferCache *cache = buffer_cache_get(); - VipsBufferCacheList *cache_list; + VipsBufferCache *cache = buffer_cache_get( im ); + VipsBuffer *buffer; GSList *p; VipsRect *area; - cache_list = g_hash_table_lookup( cache->hash, im ); - p = cache_list ? cache_list->buffers : NULL; - /* This needs to be quick :-( don't use * vips_slist_map2()/vips_rect_includesrect(), do the search inline. * * FIXME we return the first enclosing buffer, perhaps we should * search for the largest? */ - for( ; p; p = p->next ) { + for( p = cache->buffers; p; p = p->next ) { buffer = (VipsBuffer *) p->data; area = &buffer->area; @@ -456,17 +452,15 @@ buffer_find( VipsImage *im, VipsRect *r ) buffer ); #endif /*DEBUG_VERBOSE*/ - break; + return( buffer ); } } - if( p ) - return( buffer ); - else - return( NULL ); + return( NULL ); } -/* Return a ref to a buffer that encloses area. +/* Return a ref to a buffer that encloses area. The buffer we return might be + * done. */ VipsBuffer * vips_buffer_ref( VipsImage *im, VipsRect *area ) @@ -548,13 +542,13 @@ vips__buffer_init( void ) { #ifdef HAVE_PRIVATE_INIT static GPrivate private = - G_PRIVATE_INIT( (GDestroyNotify) buffer_cache_free ); + G_PRIVATE_INIT( (GDestroyNotify) buffer_thread_free ); - thread_buffer_cache_key = &private; + buffer_thread_key = &private; #else - if( !thread_buffer_cache_key ) - thread_buffer_cache_key = g_private_new( - (GDestroyNotify) buffer_cache_free ); + if( !buffer_thread_key ) + buffer_thread_key = g_private_new( + (GDestroyNotify) buffer_thread_free ); #endif if( buffer_cache_max_reserve < 1 ) @@ -564,10 +558,10 @@ vips__buffer_init( void ) void vips__buffer_shutdown( void ) { - VipsBufferCache *cache; + VipsBufferThread *buffer_thread; - if( (cache = g_private_get( thread_buffer_cache_key )) ) { - buffer_cache_free( cache ); - g_private_set( thread_buffer_cache_key, NULL ); + if( (buffer_thread = g_private_get( buffer_thread_key )) ) { + buffer_thread_free( buffer_thread ); + g_private_set( buffer_thread_key, NULL ); } } diff --git a/tools/vipsprofile b/tools/vipsprofile index 7702cfa2..ba88020c 100644 --- a/tools/vipsprofile +++ b/tools/vipsprofile @@ -223,6 +223,15 @@ def is_overlap(events, gate_name1, gate_name2): if event2.gate_name != gate_name2: continue + # events are sorted by start time, so if we've gone past event1's + # stop time, we can give up + if event2.start > event1.stop: + break + + # ... or if we're before event1's start + if event2.stop < event1.start: + continue + # if either endpoint of 1 is within 2 if event1.start > event2.start and event1.stop < event2.stop: return True