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

@@ -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. 

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 */
+} VipsBufferThread;
-	int n_reserve;		/* Number in reserve */
-} VipsBufferCache;
 
 /* 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;
+	VipsBufferThread *buffer_thread;
-} VipsBufferCacheList;
+	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;

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
+/* The maximum numbers of buffers we hold in reserve per image. 
- * enough to stop malloc cycling on vips_sharpen().
  */
-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
+	g_free( cache );
-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 );
 }
 
 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, 
+	cache->buffers = NULL;
-		NULL, (GDestroyNotify) buffer_cache_list_free );
 	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 VipsBufferThread *
- */
+buffer_thread_new( void )
-static VipsBufferCache *
-buffer_cache_get( void )
 {
-	VipsBufferCache *cache;
+	VipsBufferThread *buffer_thread;
 
-	if( !(cache = g_private_get( thread_buffer_cache_key )) ) {
+	buffer_thread = g_new( VipsBufferThread, 1 );
-		cache = buffer_cache_new();
+	buffer_thread->hash = g_hash_table_new_full( 
-		g_private_set( thread_buffer_cache_key, cache );
+		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();
+		VipsBufferCache *cache = buffer_cache_get( im ); 
-		VipsBufferCacheList *cache_list;
 
 #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. 
+		g_assert( !g_slist_find( cache->buffers, buffer ) );
-		 */
+		g_assert( !buffer->cache ); 
-		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_list->buffers, buffer ) );
+		cache->buffers = g_slist_prepend( cache->buffers, buffer );
-		g_assert( cache_list->thread == cache->thread );
 
-		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();
+	VipsBufferCache *cache = buffer_cache_get( im );
-	VipsBufferCacheList *cache_list;
+
 	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( NULL );
-		return( buffer );
-	else
-		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 ) 
+	if( !buffer_thread_key ) 
-		thread_buffer_cache_key = g_private_new( 
+		buffer_thread_key = g_private_new( 
-			(GDestroyNotify) buffer_cache_free );
+			(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 )) ) {
+	if( (buffer_thread = g_private_get( buffer_thread_key )) ) {
-		buffer_cache_free( cache );
+		buffer_thread_free( buffer_thread );
-		g_private_set( thread_buffer_cache_key, NULL );
+		g_private_set( buffer_thread_key, NULL );
 	}
 }

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