From 7938903d22a9f38d4baa0280cc2420fc6d3370d6 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Sun, 23 Apr 2017 08:31:16 +0100 Subject: [PATCH] don't size caches by image width we were sizing buffers partly by image width, which could cause caches to be too small if width changed down a pipeline see https://github.com/jcupitt/libvips/issues/639 --- ChangeLog | 3 +++ configure.ac | 6 +++--- libvips/include/vips/threadpool.h | 5 +++++ libvips/iofuncs/sinkdisc.c | 13 +++++++++---- libvips/iofuncs/threadpool.c | 29 ++++++++++++++++++++++++++++- 5 files changed, 48 insertions(+), 8 deletions(-) diff --git a/ChangeLog b/ChangeLog index fa84c42c..3611171e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,6 @@ +23/4/17 started 8.5.4 +- don't depend on image width when setting n_lines + 7/4/17 started 8.5.3 - more link fixing in docs - revise cache sizing again to help out of order errors under heavy load, thanks diff --git a/configure.ac b/configure.ac index 3aa57c61..77db0091 100644 --- a/configure.ac +++ b/configure.ac @@ -2,7 +2,7 @@ # also update the version number in the m4 macros below -AC_INIT([vips], [8.5.3], [vipsip@jiscmail.ac.uk]) +AC_INIT([vips], [8.5.4], [vipsip@jiscmail.ac.uk]) # required for gobject-introspection AC_PREREQ(2.62) @@ -18,7 +18,7 @@ AC_CONFIG_MACRO_DIR([m4]) # user-visible library versioning m4_define([vips_major_version], [8]) m4_define([vips_minor_version], [5]) -m4_define([vips_micro_version], [3]) +m4_define([vips_micro_version], [4]) m4_define([vips_version], [vips_major_version.vips_minor_version.vips_micro_version]) @@ -38,7 +38,7 @@ VIPS_VERSION_STRING=$VIPS_VERSION-`date` # binary interface changes not backwards compatible?: reset age to 0 LIBRARY_CURRENT=49 -LIBRARY_REVISION=3 +LIBRARY_REVISION=4 LIBRARY_AGE=7 # patched into include/vips/version.h diff --git a/libvips/include/vips/threadpool.h b/libvips/include/vips/threadpool.h index add66565..8adb97b2 100644 --- a/libvips/include/vips/threadpool.h +++ b/libvips/include/vips/threadpool.h @@ -88,6 +88,11 @@ typedef struct _VipsThreadState { */ void *a; + /* Set in allocate to stall this thread for a moment. Handy for + * debugging race conditions. + */ + gboolean stall; + } VipsThreadState; typedef struct _VipsThreadStateClass { diff --git a/libvips/iofuncs/sinkdisc.c b/libvips/iofuncs/sinkdisc.c index 30f2134b..b0ac977e 100644 --- a/libvips/iofuncs/sinkdisc.c +++ b/libvips/iofuncs/sinkdisc.c @@ -331,6 +331,10 @@ wbuffer_allocate_fn( VipsThreadState *state, void *a, gboolean *stop ) sink_base->y += sink_base->tile_height; if( sink_base->y >= VIPS_RECT_BOTTOM( &write->buf->area ) ) { + VIPS_DEBUG_MSG( "wbuffer_allocate_fn: " + "finished top = %d, height = %d\n", + write->buf->area.top, write->buf->area.height ); + /* Block until the write of the previous buffer * is done, then set write of this buffer going. */ @@ -346,10 +350,6 @@ wbuffer_allocate_fn( VipsThreadState *state, void *a, gboolean *stop ) return( 0 ); } - VIPS_DEBUG_MSG( "wbuffer_allocate_fn: " - "finished top = %d, height = %d\n", - write->buf->area.top, write->buf->area.height ); - VIPS_DEBUG_MSG( "wbuffer_allocate_fn: " "starting top = %d, height = %d\n", sink_base->y, sink_base->nlines ); @@ -365,6 +365,11 @@ wbuffer_allocate_fn( VipsThreadState *state, void *a, gboolean *stop ) *stop = TRUE; return( -1 ); } + + /* This will be the first tile of a new buffer ... + * stall for a moment to stress the caching system. + */ + state->stall = TRUE; } } diff --git a/libvips/iofuncs/threadpool.c b/libvips/iofuncs/threadpool.c index ef74a7fa..149c0de3 100644 --- a/libvips/iofuncs/threadpool.c +++ b/libvips/iofuncs/threadpool.c @@ -18,6 +18,9 @@ * 6/3/17 * - remove single-thread-first-request thing, new seq system makes it * unnecessary + * 23/4/17 + * - add ->stall + * - don't depend on image width when setting n_lines */ /* @@ -111,6 +114,10 @@ int vips__n_active_threads = 0; */ static GPrivate *is_worker_key = NULL; +/* Set to stall threads for debugging. + */ +static gboolean vips__stall = FALSE; + /* Glib 2.32 revised the thread API. We need some compat functions. */ @@ -466,6 +473,7 @@ vips_thread_state_init( VipsThreadState *state ) state->reg = NULL; state->stop = FALSE; + state->stall = FALSE; } void * @@ -634,6 +642,17 @@ vips_thread_work_unit( VipsThread *thr ) g_mutex_unlock( pool->allocate_lock ); + if( thr->state->stall && + vips__stall ) { + /* Sleep for 0.5s. Handy for stressing the seq system. Stall + * is set by allocate funcs in various places. + */ + g_usleep( 500000 ); + thr->state->stall = FALSE; + printf( "vips_thread_work_unit: " + "stall done, releasing y = %d ...\n", thr->state->y ); + } + /* Process a work unit. */ if( pool->work( thr->state, pool->a ) ) { @@ -999,6 +1018,9 @@ vips__threadpool_init( void ) if( !is_worker_key ) is_worker_key = g_private_new( NULL ); #endif + + if( g_getenv( "VIPS_STALL" ) ) + vips__stall = TRUE; } /** @@ -1021,6 +1043,7 @@ vips_get_tile_size( VipsImage *im, int *tile_width, int *tile_height, int *n_lines ) { const int nthr = vips_concurrency_get(); + const int typical_image_width = 1000; /* Compiler warnings. */ @@ -1054,11 +1077,15 @@ vips_get_tile_size( VipsImage *im, * the pipeline might see a different hint and we need to synchronise * buffer sizes everywhere. * + * We also can't depend on the current image size, since that might + * change down the pipeline too. Pick a typical image width. + * * Pick the maximum buffer size we might possibly need, then round up * to a multiple of tileheight. */ *n_lines = vips__tile_height * - VIPS_ROUND_UP( vips__tile_width * nthr, im->Xsize ) / im->Xsize; + VIPS_ROUND_UP( vips__tile_width * nthr, typical_image_width ) / + typical_image_width; *n_lines = VIPS_MAX( *n_lines, vips__fatstrip_height * nthr ); *n_lines = VIPS_MAX( *n_lines, vips__thinstrip_height * nthr ); *n_lines = VIPS_ROUND_UP( *n_lines, *tile_height );