free pixel buffers on image close

We were freeing pixel buffers on thread exit. This was convenient, but
meant that main thread buffers were not freed until program exit. As a
result, long-running programs which created main thread buffers would
slowly fill the operation cache with useless junk, forcing everything else out.

This change also frees pixel buffers on image close. This makes the
cache work much better in long-running programs, and can substantially
reduce memory use.

See https://github.com/jcupitt/libvips/issues/466

ChangeLog

@@ -14,6 +14,8 @@
 - tiffsave converts for jpg if jpg compression is turned on
 - tiffsave supports --strip
 - conversions to GREY16 could lock
+- free pixel buffers on image close as well as thread exit ... stops main 
+  thread buffers clogging up the system
 
 18/5/16 started 8.3.2
 - more robust vips image reading

TODO

@@ -1,35 +1,3 @@
-- freeing buffer caches on image close does not work well, since we free regions
-  after images (strangely)
-
-  can we free images earlier?
-
-  vips_region_dispose() unrefs the buffer before it unrefs the image .. odd!
-
-- below does not work for sharp .... we must free all buffers on close
-
-- we free buffers on vips__buffer_shutdown(), called from thread shutdown,
-  but this won't happen for the main thread until program exit!
-
-  therefore any buffers allocated on the main thread will never leave cache,
-  and will eventually take over, forcing all operations out
-
-  we need to free buffers on image close, not thread exit
-
-  structure:
-
-	thread private ->
-		VipsBufferThread * -> hash(im) -> 
-			VipsBufferCache * -> 
-				array of VipsBuffer
-
-  we currently free VipsBufferThread on thread exit, instead we must free
-  VipsBufferCache on image close
-
-  alternative: keep the current thing, but keep a special note of the main 
-  thread VipsBufferCache ... on image close, remove any buffers just from that 
-
-  we'd need vips__thread_main to track the thread that called VIPS_INIT
-
 - add more webp tests to py suite
 
 - try moving some more of the CLI tests to py

libvips/include/vips/private.h

@@ -101,8 +101,17 @@ typedef struct _VipsBufferCache {
 	VipsBufferThread *buffer_thread;
 	GSList *reserve;	/* VipsBuffer kept in reserve */
 	int n_reserve;		/* Number in reserve */
+	struct _VipsBufferCacheProxy *proxy;
 } VipsBufferCache;
 
+/* A proxy object between the image and the cache. This has the lifetime of the
+ * image and keeps a nullable pointer to the buffer cache. 
+ */
+typedef struct _VipsBufferCacheProxy { 
+	VipsBufferCache *cache;
+	struct _VipsImage *im;
+} VipsBufferCacheProxy;
+
 /* What we track for each pixel buffer. These can move between caches and
  * between threads, but not between images. 
  */

libvips/iofuncs/buffer.c

@@ -20,6 +20,9 @@
  * 18/12/13
  * 	- keep a few buffers in reserve per image, stops malloc/free 
  * 	  cycling when sharing is repeatedly discovered
+ * 6/6/16
+ * 	- free buffers on image close as well as thread exit, so main thread
+ * 	  buffers don't clog up the system
  */
 
 /*
@@ -177,13 +180,20 @@ buffer_thread_free( VipsBufferThread *buffer_thread )
 	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 image is closed, buffer_cache_image_postclose() fires and will call
+ *   this
+ *
+ * These can happen in either order. 
+ */
 static void
-buffer_cache_image_postclose( VipsImage *im, VipsBufferCache *cache )
+buffer_cache_free( VipsBufferCache *cache )
 {
 	GSList *p;
 
-	printf( "buffer_cache_image_postclose: im = %p\n", im ); 
-
 #ifdef DEBUG_CREATE
 	g_mutex_lock( vips__global_lock );
 	vips__buffer_cache_all = 
@@ -196,6 +206,16 @@ buffer_cache_image_postclose( VipsImage *im, VipsBufferCache *cache )
 		g_slist_length( vips__buffer_cache_all ) );
 #endif /*DEBUG_CREATE*/
 
+	/* Need to mark undone so we don't try and take them off this cache 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;
 
@@ -203,13 +223,35 @@ buffer_cache_image_postclose( VipsImage *im, VipsBufferCache *cache )
 	}
 	VIPS_FREEF( g_slist_free, cache->reserve );
 
+	cache->proxy->cache = NULL;
+	cache->proxy = NULL;
+
 	g_free( cache );
 }
 
+static void
+buffer_cache_proxy_image_postclose( VipsImage *im, VipsBufferCacheProxy *proxy )
+{
+	g_assert( proxy->im == im );
+
+	if( proxy->cache ) {
+		VipsBufferCache *cache = proxy->cache;
+		VipsBufferThread *buffer_thread = cache->buffer_thread;
+		VipsImage *im = cache->im;
+
+		g_assert( cache->im == im );
+
+		g_hash_table_remove( buffer_thread->hash, im );
+	}
+
+	g_free( proxy ); 
+}
+
 static VipsBufferCache *
 buffer_cache_new( VipsBufferThread *buffer_thread, VipsImage *im )
 {
 	VipsBufferCache *cache;
+	VipsBufferCacheProxy *proxy;
 
 	cache = g_new( VipsBufferCache, 1 );
 	cache->buffers = NULL;
@@ -218,13 +260,18 @@ buffer_cache_new( VipsBufferThread *buffer_thread, VipsImage *im )
 	cache->buffer_thread = buffer_thread;
 	cache->reserve = NULL;
 	cache->n_reserve = 0;
+
+	proxy = g_new( VipsBufferCacheProxy, 1 );
+	proxy->im = im;
+	proxy->cache = cache;
+	cache->proxy = proxy;
 	
 	/* Free memory on image close. We can't do this on thread exit since
 	 * some buffers will be made from the main thread and that won't exit
 	 * until program termination.
 	 */
 	g_signal_connect( im, "postclose", 
-		G_CALLBACK( buffer_cache_image_postclose ), cache );
+		G_CALLBACK( buffer_cache_proxy_image_postclose ), proxy );
 
 #ifdef DEBUG_CREATE
 	g_mutex_lock( vips__global_lock );
@@ -241,22 +288,6 @@ buffer_cache_new( VipsBufferThread *buffer_thread, VipsImage *im )
 	return( cache );
 }
 
-static void
-buffer_cache_unlink( VipsBufferCache *cache )
-{
-	GSList *p;
-
-	/* Need to mark undone so we don't try and take them off this cache 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 );
-}
-
 static VipsBufferThread *
 buffer_thread_new( void )
 {
@@ -265,7 +296,7 @@ buffer_thread_new( void )
 	buffer_thread = g_new( VipsBufferThread, 1 );
 	buffer_thread->hash = g_hash_table_new_full( 
 		g_direct_hash, g_direct_equal, 
-		NULL, (GDestroyNotify) buffer_cache_unlink );
+		NULL, (GDestroyNotify) buffer_cache_free );
 	buffer_thread->thread = g_thread_self();
 
 	return( buffer_thread );