From 8790e552b64a0b86f116fb563fa871d7369b0f87 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Wed, 3 Aug 2011 10:12:45 +0100 Subject: [PATCH 1/4] fix a race in xyz to lab the table build had a race condition --- .gitignore | 1 + ChangeLog | 1 + libvips/colour/im_LabQ2disp.c | 10 +++------- libvips/colour/im_XYZ2Lab.c | 15 ++++++++++----- 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/.gitignore b/.gitignore index c6433cc5..6e004421 100644 --- a/.gitignore +++ b/.gitignore @@ -125,4 +125,5 @@ doc/reference/sgml.stamp doc/reference/xml/ doc/html doc/pdf +gtkdocerrors diff --git a/ChangeLog b/ChangeLog index 233ccfff..720aec45 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,6 +1,7 @@ 26/7/11 started 7.26.1 - doc fixups - oops, ==0 missing from a strcmp() in vips7compat +- fixed a race in im_XYZ2Lab() table build 26/7/11 started 7.26.0 - version bunp for 7.26 diff --git a/libvips/colour/im_LabQ2disp.c b/libvips/colour/im_LabQ2disp.c index d5c0a8d0..88865f93 100644 --- a/libvips/colour/im_LabQ2disp.c +++ b/libvips/colour/im_LabQ2disp.c @@ -1,10 +1,6 @@ -/* @(#) Turn Lab 32bit packed format into displayable rgb. Fast, but very - * @(#) inaccurate: for display only! - * @(#) - * @(#) Usage: - * @(#) int im_LabQ2disp( IMAGE *in, IMAGE *out, struct im_col_display *d ) - * @(#) - * @(#) Returns: -1 on error, else 0 +/* Turn Lab 32bit packed format into displayable rgb. Fast, but very + * inaccurate: for display only! Note especially that this dithers and will + * give different results on different runs. * * 5/11/97 Steve Perry * - adapted from old ip code diff --git a/libvips/colour/im_XYZ2Lab.c b/libvips/colour/im_XYZ2Lab.c index 2c936ba7..7a5892f4 100644 --- a/libvips/colour/im_XYZ2Lab.c +++ b/libvips/colour/im_XYZ2Lab.c @@ -15,6 +15,8 @@ * - ahem, build the LUT outside the eval thread * 2/11/09 * - gtkdoc + * 3/8/11 + * - fix a race in the table build */ /* @@ -74,15 +76,14 @@ imb_XYZ2Lab_tables( void ) { static int built_tables = 0; - int was_built; int i; g_mutex_lock( im__global_lock ); - was_built = built_tables; - built_tables = 1; - g_mutex_unlock( im__global_lock ); - if( was_built ) + + if( built_tables ) { + g_mutex_unlock( im__global_lock ); return; + } for( i = 0; i < QUANT_ELEMENTS; i++ ) { float Y = (double) i / QUANT_ELEMENTS; @@ -92,6 +93,10 @@ imb_XYZ2Lab_tables( void ) else cbrt_table[i] = cbrt( Y ); } + + built_tables = 1; + + g_mutex_unlock( im__global_lock ); } /* Process a buffer of data. From a848adc7b31c6318292e35950675e5b32c2144e9 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Wed, 3 Aug 2011 11:17:57 +0100 Subject: [PATCH 2/4] add im_concurrency_get() to operation db to help benchmarkn.sh loop appropriately automatically --- ChangeLog | 1 + libvips/deprecated/package.c | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/ChangeLog b/ChangeLog index 720aec45..f7a4c9e9 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,7 @@ - doc fixups - oops, ==0 missing from a strcmp() in vips7compat - fixed a race in im_XYZ2Lab() table build +- added im_concurrency_get() to operation db 26/7/11 started 7.26.0 - version bunp for 7.26 diff --git a/libvips/deprecated/package.c b/libvips/deprecated/package.c index b63bc620..94c6141e 100644 --- a/libvips/deprecated/package.c +++ b/libvips/deprecated/package.c @@ -435,6 +435,35 @@ static im_function version_desc = { version_args /* Arg list */ }; +/* im_concurrency_get() args. + */ +static im_arg_desc concurrency_get_args[] = { + IM_OUTPUT_INT( "concurrency" ) +}; + +/* Call im_concurrency_get() via arg vector. + */ +static int +concurrency_get_vec( im_object *argv ) +{ + int *out = ((int *) argv[0]); + + *out = vips_concurrency_get(); + + return( 0 ); +} + +/* Description of im_concurrency_get. + */ +static im_function concurrency_get_desc = { + "im_concurrency_get", /* Name */ + "get concurrency level", /* Description */ + 0, /* Flags */ + concurrency_get_vec, /* Dispatch function */ + VIPS_NUMBER( concurrency_get_args ), /* Size of arg list */ + concurrency_get_args /* Arg list */ +}; + /* im_cache() args. */ static im_arg_desc cache_args[] = { @@ -518,6 +547,7 @@ static im_function binfile_desc = { static im_function *iofuncs_list[] = { &binfile_desc, &cache_desc, + &concurrency_get_desc, &getext_desc, &guess_prefix_desc, &guess_libdir_desc, From 764ce559f812b72e8b932b585a6c407ff55f50f1 Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Wed, 3 Aug 2011 11:25:31 +0100 Subject: [PATCH 3/4] better benchmark prog the benchmark program (benchmark/benchmarkn.sh) runs each test three times and just reports the fastest run it also detects the number of CPUs you have and automatically loops the right number of times additionally, tiles now default to 512x512, so it explicitly sets tiles back to 64x64 to make comparisons with earlier versions easier --- ChangeLog | 2 ++ benchmark/benchmarkn.sh | 50 ++++++++++++++++++++++++++++------------- 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/ChangeLog b/ChangeLog index f7a4c9e9..4869fdfd 100644 --- a/ChangeLog +++ b/ChangeLog @@ -3,6 +3,8 @@ - oops, ==0 missing from a strcmp() in vips7compat - fixed a race in im_XYZ2Lab() table build - added im_concurrency_get() to operation db +- better benchmarkn.sh runs for the correct number of CPUs automatically, runs + three times for each one, and just reports the fastest 26/7/11 started 7.26.0 - version bunp for 7.26 diff --git a/benchmark/benchmarkn.sh b/benchmark/benchmarkn.sh index 16a40b63..9527cda4 100755 --- a/benchmark/benchmarkn.sh +++ b/benchmark/benchmarkn.sh @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash uname -a gcc --version @@ -7,7 +7,7 @@ vips --version # how large an image do you want to process? # sample2.v is 290x442 pixels ... replicate this many times horizontally and # vertically to get a highres image for the benchmark -tile=2 +tile=13 # how complex an operation do you want to run? # this sets the number of copies of the benchmark we chain together: @@ -21,26 +21,44 @@ if [ $? != 0 ]; then echo "build of test image failed -- out of disc space?" exit 1 fi -echo -n "test image is" `header -f Xsize temp.v` -echo " by" `header -f Ysize temp.v` "pixels" +echo -n "test image is" `header -f width temp.v` +echo " by" `header -f height temp.v` "pixels" +max_cpus=`vips im_concurrency_get` +echo "max cpus = $max_cpus" echo "starting benchmark ..." -echo "chain=$chain" - -for cpus in 1 2 3 4 ; do - export IM_CONCURRENCY=$cpus - - echo IM_CONCURRENCY=$IM_CONCURRENCY - echo time -p vips im_benchmarkn temp.v temp2.v $chain - time -p vips im_benchmarkn temp.v temp2-$cpus.v $chain +echo /usr/bin/time -f %e vips \ + --vips-concurrency=xx \ + --vips-tile-width=64 --vips-tile-height=64 \ + im_benchmarkn temp.v temp2.v $chain +echo reported real-time is best of three runs +echo cpus real-time +for((cpus = 1; cpus <= max_cpus; cpus++)); do + t1=`/usr/bin/time -f %e vips \ + --vips-concurrency=$cpus \ + --vips-tile-width=64 --vips-tile-height=64 \ + im_benchmarkn temp.v temp2.v $chain 2>&1` if [ $? != 0 ]; then echo "benchmark failed -- install problem?" exit 1 fi + t2=`/usr/bin/time -f %e vips \ + --vips-concurrency=$cpus \ + --vips-tile-width=64 --vips-tile-height=64 \ + im_benchmarkn temp.v temp2.v $chain 2>&1` + t3=`/usr/bin/time -f %e vips \ + --vips-concurrency=$cpus \ + --vips-tile-width=64 --vips-tile-height=64 \ + im_benchmarkn temp.v temp2.v $chain 2>&1` - # find pixel average ... should be the same for all IM_CONCURRENCY settings - # or we have some kind of terrible bug - echo vips im_avg temp2-$cpus.v - vips im_avg temp2-$cpus.v + # echo $t1 $t2 $t3 + + if [[ $t2 < $t1 ]]; then + t1=$t2 + fi + if [[ $t3 < $t1 ]]; then + t1=$t3 + fi + echo $cpus $t1 done From f5397a68af416a26a6b4c0fb258bd1abd627a3fe Mon Sep 17 00:00:00 2001 From: John Cupitt Date: Tue, 9 Aug 2011 11:15:09 +0100 Subject: [PATCH 4/4] sync --- TODO | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/TODO b/TODO index 60b57013..a8787798 100644 --- a/TODO +++ b/TODO @@ -1,21 +1,4 @@ -- in benchmark, try: - vips im_benchmarkn temp.v temp2.v 1 - vips --vips-concurrency=99 --vips-tile-width=10 --vips-tile-height=10 \ - im_benchmarkn temp.v temp2a.v 1 - vips im_avg temp2a.v - vips im_avg temp2.v - - get different results :-( - - 7.24 seems to fail this test too argh - - first column of tiles is fine, but stuff to the right of that is broken - - - - -- leak check, again - vips operation print could show operation flags as well, cf. "vips im_subtract" @@ -94,7 +77,6 @@ - do we bundle "convert" in the OS X / win32 builds? if we don't we should - - some way for nip2 to get the vips bin area