From a8d04a7dd19ea8057ad01c284c7baa91a0726505 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 12 Jun 2014 09:22:28 +0100 Subject: [PATCH 1/8] add op invalidate stuff still need to test and link to cache --- ChangeLog | 1 + TODO | 8 +++++ libvips/include/vips/object.h | 5 +++ libvips/include/vips/operation.h | 6 ++++ libvips/iofuncs/object.c | 56 +++++++++++++++++++++++--------- libvips/iofuncs/operation.c | 26 +++++++++++++++ 6 files changed, 86 insertions(+), 16 deletions(-) diff --git a/ChangeLog b/ChangeLog index 8f0b3c80..d14b06c5 100644 --- a/ChangeLog +++ b/ChangeLog @@ -37,6 +37,7 @@ im_*mosaic1(), im_*merge1() redone as classes - 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 6/3/14 started 7.38.6 - grey ramp minimum was wrong diff --git a/TODO b/TODO index 0f9a9b91..733c2be3 100644 --- a/TODO +++ b/TODO @@ -1,3 +1,11 @@ +- 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/object.h b/libvips/include/vips/object.h index d16d16f5..e94c7de8 100644 --- a/libvips/include/vips/object.h +++ b/libvips/include/vips/object.h @@ -283,6 +283,11 @@ typedef struct _VipsArgumentInstance { * here. */ gulong close_id; + + /* We need to listen for "invalidate" on input images and send our own + * "invalidate" out. If we go, we need to disconnect. + */ + gulong invalidate_id; } VipsArgumentInstance; /* Need to look up our VipsArgument structs from a pspec. Just hash the diff --git a/libvips/include/vips/operation.h b/libvips/include/vips/operation.h index 0afb9a9d..acc9f354 100644 --- a/libvips/include/vips/operation.h +++ b/libvips/include/vips/operation.h @@ -92,12 +92,18 @@ typedef struct _VipsOperationClass { */ VipsOperationFlags (*get_flags)( VipsOperation * ); VipsOperationFlags flags; + + /* One of our input images has signalled "invalidate". The cache uses + * VipsOperation::invalidate to drop dirty ops. + */ + void (*invalidate)( VipsOperation * ); } VipsOperationClass; GType vips_operation_get_type( void ); VipsOperationFlags vips_operation_get_flags( VipsOperation *operation ); void vips_operation_class_print_usage( VipsOperationClass *operation_class ); +void vips_operation_invalidate( VipsOperation *operation ); int vips_operation_call_valist( VipsOperation *operation, va_list ap ); VipsOperation *vips_operation_new( const char *name ); diff --git a/libvips/iofuncs/object.c b/libvips/iofuncs/object.c index cf4176eb..1b2136be 100644 --- a/libvips/iofuncs/object.c +++ b/libvips/iofuncs/object.c @@ -392,16 +392,34 @@ vips_object_rewind( VipsObject *object ) /* Extra stuff we track for properties to do our argument handling. */ +static void +vips_argument_instance_detach( VipsArgumentInstance *argument_instance ) +{ + VipsObject *object = argument_instance->object; + + if( argument_instance->close_id ) { + if( g_signal_handler_is_connected( object, + argument_instance->close_id ) ) + g_signal_handler_disconnect( object, + argument_instance->close_id ); + argument_instance->close_id = 0; + } + + if( argument_instance->invalidate_id ) { + if( g_signal_handler_is_connected( object, + argument_instance->invalidate_id ) ) + g_signal_handler_disconnect( object, + argument_instance->invalidate_id ); + argument_instance->invalidate_id = 0; + } +} + /* Free a VipsArgumentInstance ... VipsArgumentClass can just be g_free()d. */ static void vips_argument_instance_free( VipsArgumentInstance *argument_instance ) { - if( argument_instance->close_id ) { - g_signal_handler_disconnect( argument_instance->object, - argument_instance->close_id ); - argument_instance->close_id = 0; - } + vips_argument_instance_detach( argument_instance ); g_free( argument_instance ); } @@ -580,6 +598,7 @@ vips_argument_init( VipsObject *object ) argument_class->flags & VIPS_ARGUMENT_SET_ALWAYS; argument_instance->close_id = 0; + argument_instance->invalidate_id = 0; vips_argument_table_replace( object->argument_table, (VipsArgument *) argument_instance ); @@ -729,6 +748,8 @@ vips_object_clear_member( VipsObject *object, GParamSpec *pspec, VipsArgumentInstance *argument_instance = vips__argument_get_instance( argument_class, object ); + vips_argument_instance_detach( argument_instance ); + if( *member ) { if( argument_class->flags & VIPS_ARGUMENT_INPUT ) { #ifdef DEBUG_REF @@ -754,17 +775,6 @@ vips_object_clear_member( VipsObject *object, GParamSpec *pspec, G_OBJECT( object )->ref_count - 1 ); #endif /*DEBUG_REF*/ - /* The object reffed us. Stop listening link to the - * object's "close" signal. We can come here from - * object being closed, in which case the handler - * will already have been disconnected for us. - */ - if( g_signal_handler_is_connected( object, - argument_instance->close_id ) ) - g_signal_handler_disconnect( object, - argument_instance->close_id ); - argument_instance->close_id = 0; - g_object_unref( object ); } @@ -919,6 +929,13 @@ vips_object_finalize( GObject *gobject ) G_OBJECT_CLASS( vips_object_parent_class )->finalize( gobject ); } +static void +vips_object_arg_invalidate( GObject *argument, + VipsArgumentInstance *argument_instance ) +{ + vips_operation_invalidate( VIPS_OPERATION( argument ) ); +} + static void vips_object_arg_close( GObject *argument, VipsArgumentInstance *argument_instance ) @@ -968,6 +985,13 @@ vips__object_set_member( VipsObject *object, GParamSpec *pspec, /* Ref the argument. */ g_object_ref( *member ); + + g_assert( !argument_instance->invalidate_id ); + argument_instance->invalidate_id = + g_signal_connect( *member, "invalidate", + G_CALLBACK( + vips_object_arg_invalidate ), + argument_instance ); } else if( argument_class->flags & VIPS_ARGUMENT_OUTPUT ) { #ifdef DEBUG_REF diff --git a/libvips/iofuncs/operation.c b/libvips/iofuncs/operation.c index cdfd4285..9cc4ab92 100644 --- a/libvips/iofuncs/operation.c +++ b/libvips/iofuncs/operation.c @@ -92,6 +92,15 @@ /* Abstract base class for operations. */ +/* Our signals. + */ +enum { + SIG_INVALIDATE, + SIG_LAST +}; + +static guint vips_operation_signals[SIG_LAST] = { 0 }; + G_DEFINE_ABSTRACT_TYPE( VipsOperation, vips_operation, VIPS_TYPE_OBJECT ); static void @@ -380,6 +389,14 @@ vips_operation_class_init( VipsOperationClass *class ) class->usage = vips_operation_usage; class->get_flags = vips_operation_real_get_flags; + + vips_operation_signals[SIG_INVALIDATE] = g_signal_new( "invalidate", + G_TYPE_FROM_CLASS( class ), + G_SIGNAL_RUN_LAST, + G_STRUCT_OFFSET( VipsOperationClass, invalidate ), + NULL, NULL, + g_cclosure_marshal_VOID__VOID, + G_TYPE_NONE, 0 ); } static void @@ -419,6 +436,15 @@ vips_operation_class_print_usage( VipsOperationClass *operation_class ) printf( "%s", vips_buf_all( &buf ) ); } +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 ); +} + VipsOperation * vips_operation_new( const char *name ) { From 66425bec8e769c4b6c8ea85ed00c66f577ad5f32 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 12 Jun 2014 12:59:53 +0100 Subject: [PATCH 2/8] fix up operation invalidate cache drop next --- libvips/iofuncs/object.c | 63 ++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/libvips/iofuncs/object.c b/libvips/iofuncs/object.c index 1b2136be..00fdc59a 100644 --- a/libvips/iofuncs/object.c +++ b/libvips/iofuncs/object.c @@ -396,19 +396,22 @@ static void vips_argument_instance_detach( VipsArgumentInstance *argument_instance ) { VipsObject *object = argument_instance->object; + VipsArgumentClass *argument_class = argument_instance->argument_class; + GObject *member = G_STRUCT_MEMBER( GObject *, object, + argument_class->offset ); if( argument_instance->close_id ) { - if( g_signal_handler_is_connected( object, + if( g_signal_handler_is_connected( member, argument_instance->close_id ) ) - g_signal_handler_disconnect( object, + g_signal_handler_disconnect( member, argument_instance->close_id ); argument_instance->close_id = 0; } if( argument_instance->invalidate_id ) { - if( g_signal_handler_is_connected( object, + if( g_signal_handler_is_connected( member, argument_instance->invalidate_id ) ) - g_signal_handler_disconnect( object, + g_signal_handler_disconnect( member, argument_instance->invalidate_id ); argument_instance->invalidate_id = 0; } @@ -739,14 +742,12 @@ vips_object_get_argument_priority( VipsObject *object, const char *name ) } static void -vips_object_clear_member( VipsObject *object, GParamSpec *pspec, - GObject **member ) +vips_object_clear_member( VipsArgumentInstance *argument_instance ) { - VipsObjectClass *class = VIPS_OBJECT_GET_CLASS( object ); - VipsArgumentClass *argument_class = (VipsArgumentClass *) - vips__argument_table_lookup( class->argument_table, pspec ); - VipsArgumentInstance *argument_instance = - vips__argument_get_instance( argument_class, object ); + VipsObject *object = argument_instance->object; + VipsArgumentClass *argument_class = argument_instance->argument_class; + GObject **member = &G_STRUCT_MEMBER( GObject *, object, + argument_class->offset ); vips_argument_instance_detach( argument_instance ); @@ -933,7 +934,12 @@ static void vips_object_arg_invalidate( GObject *argument, VipsArgumentInstance *argument_instance ) { - vips_operation_invalidate( VIPS_OPERATION( argument ) ); + /* Image @argument has signalled "invalidate" ... resignal on our + * operation. + */ + if( VIPS_IS_OPERATION( argument_instance->object ) ) + vips_operation_invalidate( + VIPS_OPERATION( argument_instance->object ) ); } static void @@ -963,10 +969,11 @@ vips__object_set_member( VipsObject *object, GParamSpec *pspec, vips__argument_table_lookup( class->argument_table, pspec ); VipsArgumentInstance *argument_instance = vips__argument_get_instance( argument_class, object ); + GType otype = G_PARAM_SPEC_VALUE_TYPE( pspec ); g_assert( argument_instance ); - vips_object_clear_member( object, pspec, member ); + vips_object_clear_member( argument_instance ); g_assert( !*member ); *member = argument; @@ -974,7 +981,7 @@ vips__object_set_member( VipsObject *object, GParamSpec *pspec, if( *member ) { if( argument_class->flags & VIPS_ARGUMENT_INPUT ) { #ifdef DEBUG_REF - printf( "vips_object_set_member: vips object: " ); + printf( "vips__object_set_member: vips object: " ); vips_object_print_name( object ); printf( " refers to gobject %s (%p)\n", G_OBJECT_TYPE_NAME( *member ), *member ); @@ -985,17 +992,10 @@ vips__object_set_member( VipsObject *object, GParamSpec *pspec, /* Ref the argument. */ g_object_ref( *member ); - - g_assert( !argument_instance->invalidate_id ); - argument_instance->invalidate_id = - g_signal_connect( *member, "invalidate", - G_CALLBACK( - vips_object_arg_invalidate ), - argument_instance ); } else if( argument_class->flags & VIPS_ARGUMENT_OUTPUT ) { #ifdef DEBUG_REF - printf( "vips_object_set_member: gobject %s (%p)\n", + printf( "vips__object_set_member: gobject %s (%p)\n", G_OBJECT_TYPE_NAME( *member ), *member ); printf( " refers to vips object: " ); vips_object_print_name( object ); @@ -1006,10 +1006,23 @@ vips__object_set_member( VipsObject *object, GParamSpec *pspec, /* The argument reffs us. */ g_object_ref( object ); + } + } - /* FIXME ... could use a NULLing weakref - */ + if( *member && + g_type_is_a( otype, VIPS_TYPE_IMAGE ) ) { + if( argument_class->flags & VIPS_ARGUMENT_INPUT ) { + g_assert( !argument_instance->invalidate_id ); + + argument_instance->invalidate_id = + g_signal_connect( *member, "invalidate", + G_CALLBACK( + vips_object_arg_invalidate ), + argument_instance ); + } + else if( argument_class->flags & VIPS_ARGUMENT_OUTPUT ) { g_assert( !argument_instance->close_id ); + argument_instance->close_id = g_signal_connect( *member, "close", G_CALLBACK( vips_object_arg_close ), @@ -1122,7 +1135,7 @@ vips_object_set_property( GObject *gobject, GObject **member = &G_STRUCT_MEMBER( GObject *, object, argument_class->offset ); - vips__object_set_member( object, pspec, member, + vips__object_set_member( object, pspec, member, g_value_get_object( value ) ); } else if( G_IS_PARAM_SPEC_INT( pspec ) ) { From d7bad8fd5bcf36f200b3a81bf8ba713245e0a2c6 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 12 Jun 2014 13:40:00 +0100 Subject: [PATCH 3/8] 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 ); } From 40e8025a553d988512bb86a8d91e33a1ebab4fa1 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 12 Jun 2014 14:23:53 +0100 Subject: [PATCH 4/8] more fixups to pass testsuite --- libvips/iofuncs/cache.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/libvips/iofuncs/cache.c b/libvips/iofuncs/cache.c index 811c496f..2d6c6a59 100644 --- a/libvips/iofuncs/cache.c +++ b/libvips/iofuncs/cache.c @@ -464,10 +464,12 @@ vips__cache_init( void ) static void * vips_cache_print_fn( void *value, void *a, void *b ) { + VipsOperationCacheEntry *entry = value; + char str[32768]; VipsBuf buf = VIPS_BUF_STATIC( str ); - vips_object_to_string( VIPS_OBJECT( value ), &buf ); + vips_object_to_string( VIPS_OBJECT( entry->operation ), &buf ); printf( "%p - %s\n", value, vips_buf_all( &buf ) ); @@ -539,13 +541,14 @@ vips_cache_remove( VipsOperation *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_hash_table_remove( vips_cache_table, operation ); + vips_cache_unref( operation ); + g_free( entry ); } @@ -626,11 +629,14 @@ vips_cache_get_first_fn( void *value, void *a, void *b ) static VipsOperation * vips_cache_get_first( void ) { - if( vips_cache_table ) - return( VIPS_OPERATION( vips_hash_table_map( vips_cache_table, - vips_cache_get_first_fn, NULL, NULL ) ) ); - else - return( NULL ); + VipsOperationCacheEntry *entry; + + if( vips_cache_table && + (entry = vips_hash_table_map( vips_cache_table, + vips_cache_get_first_fn, NULL, NULL )) ) + return( VIPS_OPERATION( entry->operation ) ); + + return( NULL ); } /** From 7bc0ca728310d6a9f19e988e8a898fd4f79a26c5 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 12 Jun 2014 14:35:38 +0100 Subject: [PATCH 5/8] update notes --- TODO | 57 +-------------------------------------------------------- 1 file changed, 1 insertion(+), 56 deletions(-) diff --git a/TODO b/TODO index 0f9a9b91..c5ec0a6c 100644 --- a/TODO +++ b/TODO @@ -1,4 +1,3 @@ - - can we use postbuild elsewhere? look at use of "preclose" / "written", etc. @@ -23,42 +22,6 @@ -- think of a better way to support skipahead - - extract needs to hint to it's image sources what line it will start reading - at - - in vips_extract_build(), call vips_image_set_start(), somehow sends a signal - the wrong way down the pipe to the vips_sequential() (if there is one) - telling it not to stall the first request if it's for that line - - test --crop in vipsthumbnail on portrait images cropped to landscape - - alternative: if extract sees a seq image, it deliberately reads and discards - the N scanlines - - could be very slow :-( - - the first vips_extract_area_gen() needs to somehow signal in - vips_region_prepare() that this is the first request - - how do we pass the signal down? not clear - - run the first tile single-threaded - - so sinkdisc runs the first request, waits for it to come back, then - starts all the workers going - - this is quite a good idea! we'd not slow down much, and there's a huge - amount of locking on the first tile anyway - - now seq can support skipahead again - - - - - - - now vips_linear() has uchar output, can we do something with orc? - do restrict on some more packages, we've just done arithmetic so far @@ -98,6 +61,7 @@ - use g_log() instead of vips_info()? + - quadratic doesn't work for order 3 start to get jaggies on lines --- the 3rd differential isn't being @@ -108,18 +72,6 @@ because we step across tiles left to right: y doesn't change, only x does - -- the operation cache needs to detect invalidate - - tricky! - - perhaps have operations always watching all of their inputs and resignalling - "invalidate" themselves - - cache then just needs to watch for "invalidate" on operations it tracks - - need to add an "invalidate" signal to operation - mosaic ====== @@ -185,9 +137,6 @@ packaging - test _O_TEMPORARY thing on Windows -- do we bundle "convert" in the OS X / win32 builds? if we don't we - should - convolution =========== @@ -224,10 +173,6 @@ convolution arithmetic ========== -- avg/dev etc. should uncode images? eg. labq2lab etc. - - how about ifthenelse? - - HAVE_HYPOT could define a hypot() macro? - fix a better NaN policy From bf56f8f2033f80e09fbc8986e888eee745fbaf05 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Thu, 12 Jun 2014 14:41:04 +0100 Subject: [PATCH 6/8] oops, missed another NULL --- libvips/iofuncs/cache.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/libvips/iofuncs/cache.c b/libvips/iofuncs/cache.c index 2d6c6a59..15308214 100644 --- a/libvips/iofuncs/cache.c +++ b/libvips/iofuncs/cache.c @@ -690,7 +690,10 @@ vips_cache_get_lru( void ) g_hash_table_foreach( vips_cache_table, (GHFunc) vips_cache_get_lru_cb, &entry ); - return( entry->operation ); + if( entry ) + return( entry->operation ); + + return( NULL ); } /* Is the cache full? Drop until it's not. From 6e48c475336ebcbf86fdb29fdc9585b7a25eba8b Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Fri, 13 Jun 2014 09:44:58 +0100 Subject: [PATCH 7/8] stop image abuse in labelregions we were marking as image as changing by calling vips_image_readwrite(), but the cache system didn't uncache it --- TODO | 4 +++ libvips/morphology/labelregions.c | 42 ++++++++++++++----------------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/TODO b/TODO index c5ec0a6c..08afcf18 100644 --- a/TODO +++ b/TODO @@ -1,3 +1,7 @@ +- vips_image_readwrite() or whatever its called should signal invalidate or + mark the image as uncacheable or something + + - can we use postbuild elsewhere? look at use of "preclose" / "written", etc. diff --git a/libvips/morphology/labelregions.c b/libvips/morphology/labelregions.c index 4db0efee..58aa269b 100644 --- a/libvips/morphology/labelregions.c +++ b/libvips/morphology/labelregions.c @@ -60,11 +60,11 @@ static int vips_labelregions_build( VipsObject *object ) { VipsMorphology *morphology = VIPS_MORPHOLOGY( object ); - VipsLabelregions *labelregions = (VipsLabelregions *) object; VipsImage *in = morphology->in; - VipsImage **t = (VipsImage **) vips_object_local_array( object, 7 ); + VipsImage **t = (VipsImage **) vips_object_local_array( object, 2 ); + VipsImage *mask; - int serial; + int segments; int *m; int x, y; @@ -72,43 +72,39 @@ vips_labelregions_build( VipsObject *object ) build( object ) ) return( -1 ); - /* Create the zero mask image. + /* Create the zero mask image in memory. */ + mask = vips_image_new_memory(); + g_object_set( object, + "mask", mask, + NULL ); if( vips_black( &t[0], in->Xsize, in->Ysize, NULL ) || - vips_cast( t[0], &t[1], VIPS_FORMAT_INT, NULL ) ) + vips_cast( t[0], &t[1], VIPS_FORMAT_INT, NULL ) || + vips_image_write( t[1], mask ) ) return( -1 ); - /* Search the mask image, flooding as we find zero pixels. - */ - if( vips_image_inplace( t[1] ) ) - return( -1 ); - - serial = 1; - m = (int *) t[1]->data; - for( y = 0; y < t[1]->Ysize; y++ ) { - for( x = 0; x < t[1]->Xsize; x++ ) { + segments = 1; + m = (int *) mask->data; + for( y = 0; y < mask->Ysize; y++ ) { + for( x = 0; x < mask->Xsize; x++ ) { if( !m[x] ) { /* Use a direct path for speed. */ - if( vips__draw_flood_direct( t[1], in, - serial, x, y ) ) + if( vips__draw_flood_direct( mask, in, + segments, x, y ) ) return( -1 ); - serial += 1; + segments += 1; } } - m += t[1]->Xsize; + m += mask->Xsize; } g_object_set( object, - "mask", vips_image_new(), - "segments", serial, + "segments", segments, NULL ); - if( vips_image_write( t[1], labelregions->mask ) ) - return( -1 ); - return( 0 ); } From 000d77e99475227306b2f5d2bbdc0232f47fbad1 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Fri, 13 Jun 2014 10:52:07 +0100 Subject: [PATCH 8/8] final clean-up --- TODO | 2 -- libvips/iofuncs/image.c | 5 +++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/TODO b/TODO index 08afcf18..794ae38e 100644 --- a/TODO +++ b/TODO @@ -1,5 +1,3 @@ -- vips_image_readwrite() or whatever its called should signal invalidate or - mark the image as uncacheable or something - can we use postbuild elsewhere? look at use of "preclose" / "written", etc. diff --git a/libvips/iofuncs/image.c b/libvips/iofuncs/image.c index 7054b0b9..e189d184 100644 --- a/libvips/iofuncs/image.c +++ b/libvips/iofuncs/image.c @@ -2747,6 +2747,11 @@ vips_image_inplace( VipsImage *image ) return( -1 ); } + /* This image is about to be changed (probably). Make sure it's not + * in cache. + */ + vips_image_invalidate_all( image ); + return( 0 ); }