From d7bad8fd5bcf36f200b3a81bf8ba713245e0a2c6 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 12 Jun 2014 13:40:00 +0100 Subject: [PATCH] cache drops operations on invalidate we can now enable the vips8 operation cache in nip2, woo! --- ChangeLog | 1 + TODO | 8 -- libvips/include/vips/operation.h | 5 - libvips/iofuncs/cache.c | 173 +++++++++++++++++++------------ libvips/iofuncs/operation.c | 2 + 5 files changed, 111 insertions(+), 78 deletions(-) diff --git a/ChangeLog b/ChangeLog index d14b06c5..919e72de 100644 --- a/ChangeLog +++ b/ChangeLog @@ -38,6 +38,7 @@ - better filename tracking for globalbalance - revised vips8 image load/save API, now simpler and more logical - operations emit "invalidate" if any of their input images invalidate +- operation cache drops ops on invalidate 6/3/14 started 7.38.6 - grey ramp minimum was wrong diff --git a/TODO b/TODO index 733c2be3..0f9a9b91 100644 --- a/TODO +++ b/TODO @@ -1,11 +1,3 @@ -- test operation invalidate - - enable cache in nip2, find max using vips_call, try painting on the input - -- cache needs to listen for invalidate - - - - can we use postbuild elsewhere? look at use of "preclose" / "written", etc. diff --git a/libvips/include/vips/operation.h b/libvips/include/vips/operation.h index acc9f354..6f72c4b0 100644 --- a/libvips/include/vips/operation.h +++ b/libvips/include/vips/operation.h @@ -65,11 +65,6 @@ typedef gboolean (*VipsOperationBuildFn)( VipsObject * ); typedef struct _VipsOperation { VipsObject parent_instance; - /* When we added this operation to cache .. used to find LRU for - * flush. - */ - int time; - /* Keep the hash here. */ guint hash; diff --git a/libvips/iofuncs/cache.c b/libvips/iofuncs/cache.c index 3db8697c..811c496f 100644 --- a/libvips/iofuncs/cache.c +++ b/libvips/iofuncs/cache.c @@ -37,8 +37,6 @@ TODO - listen for invalidate - will we need to drop all on exit? unclear what about delayed writes ... do we ever write in close? we shouldn't, @@ -123,6 +121,22 @@ static GMutex *vips_cache_lock = NULL; #define VIPS_VALUE_GET_CHAR g_value_get_char #endif +/* A cache entry. + */ +typedef struct _VipsOperationCacheEntry { + VipsOperation *operation; + + /* When we added this operation to cache .. used to find LRU for + * flush. + */ + int time; + + /* We listen for "invalidate" from the operation. Track the id here so + * we can disconnect when we drop an operation. + */ + gulong invalidate_id; +} VipsOperationCacheEntry; + /* Pass in the pspec so we can get the generic type. For example, a * held in a GParamSpec allowing OBJECT, but the value could be of type * VipsImage. generics are much faster to compare. @@ -515,17 +529,90 @@ vips_cache_unref( VipsOperation *operation ) g_object_unref( operation ); } -/* Drop an operation from the cache. +/* Remove an operation from the cache. */ static void -vips_cache_drop( VipsOperation *operation ) +vips_cache_remove( VipsOperation *operation ) { - /* It must be in cache. - */ - g_assert( g_hash_table_lookup( vips_cache_table, operation ) ); + VipsOperationCacheEntry *entry = (VipsOperationCacheEntry *) + g_hash_table_lookup( vips_cache_table, operation ); + + g_assert( entry ); g_hash_table_remove( vips_cache_table, operation ); vips_cache_unref( operation ); + + if( entry->invalidate_id ) { + g_signal_handler_disconnect( operation, entry->invalidate_id ); + entry->invalidate_id = 0; + } + g_free( entry ); +} + +static void * +vips_object_ref_arg( VipsObject *object, + GParamSpec *pspec, + VipsArgumentClass *argument_class, + VipsArgumentInstance *argument_instance, + void *a, void *b ) +{ + if( (argument_class->flags & VIPS_ARGUMENT_CONSTRUCT) && + (argument_class->flags & VIPS_ARGUMENT_OUTPUT) && + argument_instance->assigned && + G_IS_PARAM_SPEC_OBJECT( pspec ) ) { + GObject *value; + + /* This will up the ref count for us. + */ + g_object_get( G_OBJECT( object ), + g_param_spec_get_name( pspec ), &value, NULL ); + } + + return( NULL ); +} + +static void +vips_operation_touch( VipsOperation *operation ) +{ + VipsOperationCacheEntry *entry = (VipsOperationCacheEntry *) + g_hash_table_lookup( vips_cache_table, operation ); + + vips_cache_time += 1; + entry->time = vips_cache_time; +} + +/* Ref an operation for the cache. The operation itself, plus all the output + * objects it makes. + */ +static void +vips_cache_ref( VipsOperation *operation ) +{ + g_object_ref( operation ); + (void) vips_argument_map( VIPS_OBJECT( operation ), + vips_object_ref_arg, NULL, NULL ); + vips_operation_touch( operation ); +} + +static void +vips_cache_insert( VipsOperation *operation ) +{ + VipsOperationCacheEntry *entry = g_new( VipsOperationCacheEntry, 1 ); + + /* It must not be in cache. + */ + g_assert( !g_hash_table_lookup( vips_cache_table, operation ) ); + + entry->operation = operation; + entry->time = 0; + entry->invalidate_id = 0; + + g_hash_table_insert( vips_cache_table, operation, entry ); + vips_cache_ref( operation ); + + /* If the operation signals "invalidate", we must drop it. + */ + entry->invalidate_id = g_signal_connect( operation, "invalidate", + G_CALLBACK( vips_cache_remove ), NULL ); } static void * @@ -567,7 +654,7 @@ vips_cache_drop_all( void ) * first item instead. */ while( (operation = vips_cache_get_first()) ) - vips_cache_drop( operation ); + vips_cache_remove( operation ); VIPS_FREEF( g_hash_table_unref, vips_cache_table ); } @@ -576,8 +663,8 @@ vips_cache_drop_all( void ) } static void -vips_cache_get_lru_cb( VipsOperation *key, VipsOperation *value, - VipsOperation **best ) +vips_cache_get_lru_cb( VipsOperation *key, VipsOperationCacheEntry *value, + VipsOperationCacheEntry **best ) { if( !*best || (*best)->time > value->time ) @@ -591,13 +678,13 @@ vips_cache_get_lru_cb( VipsOperation *key, VipsOperation *value, static VipsOperation * vips_cache_get_lru( void ) { - VipsOperation *operation; + VipsOperationCacheEntry *entry; - operation = NULL; + entry = NULL; g_hash_table_foreach( vips_cache_table, - (GHFunc) vips_cache_get_lru_cb, &operation ); + (GHFunc) vips_cache_get_lru_cb, &entry ); - return( operation ); + return( entry->operation ); } /* Is the cache full? Drop until it's not. @@ -618,53 +705,12 @@ vips_cache_trim( void ) printf( "vips_cache_trim: trimming %p\n", operation ); #endif /*DEBUG*/ - vips_cache_drop( operation ); + vips_cache_remove( operation ); } g_mutex_unlock( vips_cache_lock ); } -static void * -vips_object_ref_arg( VipsObject *object, - GParamSpec *pspec, - VipsArgumentClass *argument_class, - VipsArgumentInstance *argument_instance, - void *a, void *b ) -{ - if( (argument_class->flags & VIPS_ARGUMENT_CONSTRUCT) && - (argument_class->flags & VIPS_ARGUMENT_OUTPUT) && - argument_instance->assigned && - G_IS_PARAM_SPEC_OBJECT( pspec ) ) { - GObject *value; - - /* This will up the ref count for us. - */ - g_object_get( G_OBJECT( object ), - g_param_spec_get_name( pspec ), &value, NULL ); - } - - return( NULL ); -} - -static void -vips_operation_touch( VipsOperation *operation ) -{ - vips_cache_time += 1; - operation->time = vips_cache_time; -} - -/* Ref an operation for the cache. The operation itself, plus all the output - * objects it makes. - */ -static void -vips_cache_ref( VipsOperation *operation ) -{ - g_object_ref( operation ); - (void) vips_argument_map( VIPS_OBJECT( operation ), - vips_object_ref_arg, NULL, NULL ); - vips_operation_touch( operation ); -} - /** * vips_cache_operation_buildp: (skip) * @operation: pointer to operation to lookup @@ -679,7 +725,7 @@ vips_cache_ref( VipsOperation *operation ) int vips_cache_operation_buildp( VipsOperation **operation ) { - VipsOperation *hit; + VipsOperationCacheEntry *hit; g_assert( VIPS_IS_OPERATION( *operation ) ); @@ -693,15 +739,15 @@ vips_cache_operation_buildp( VipsOperation **operation ) if( (hit = g_hash_table_lookup( vips_cache_table, *operation )) ) { if( vips__cache_trace ) { printf( "vips cache-: " ); - vips_object_print_summary( VIPS_OBJECT( hit ) ); + vips_object_print_summary( VIPS_OBJECT( *operation ) ); } /* Ref before unref in case *operation == hit. */ - vips_cache_ref( hit ); + vips_cache_ref( hit->operation ); g_object_unref( *operation ); - *operation = hit; + *operation = hit->operation; } g_mutex_unlock( vips_cache_lock ); @@ -724,11 +770,8 @@ vips_cache_operation_buildp( VipsOperation **operation ) g_mutex_lock( vips_cache_lock ); if( !(vips_operation_get_flags( *operation ) & - VIPS_OPERATION_NOCACHE) ) { - vips_cache_ref( *operation ); - g_hash_table_insert( vips_cache_table, - *operation, *operation ); - } + VIPS_OPERATION_NOCACHE) ) + vips_cache_insert( *operation ); g_mutex_unlock( vips_cache_lock ); } diff --git a/libvips/iofuncs/operation.c b/libvips/iofuncs/operation.c index 9cc4ab92..716e1b38 100644 --- a/libvips/iofuncs/operation.c +++ b/libvips/iofuncs/operation.c @@ -439,8 +439,10 @@ vips_operation_class_print_usage( VipsOperationClass *operation_class ) void vips_operation_invalidate( VipsOperation *operation ) { + /* printf( "vips_operation_invalidate: %p\n", operation ); vips_object_print_summary( VIPS_OBJECT( operation ) ); + */ g_signal_emit( operation, vips_operation_signals[SIG_INVALIDATE], 0 ); }