Fix for hidden leaks in ThreadPool

[jtorresfabra]

ThreadPool was allocating memory for threads and pools locally to the image object. As that image could not be freed from cache, for each sink operation done for that image the code was creating new pools and threads without freeing the previous ones. So, the more number of threads and the more operations the bigger the hidden leak was. The fix just creates the objects not locally to that image, and also free them in its destroy functions

[jcupitt]

Looking at threadpool again, I think we should refactor a bit. For example vips_threadpool_new() will free itself if im is closed, which won't work any more with your patch. But I don't think it'll ever be used that way, so we should just get rid of the auto-free stuff.

Let's merge this, then I'll have a go at fixing up threadpool.

[jtorresfabra]

I removed that connection in the PR (https://github.com/libvips/libvips/pull/1240/files#diff-6d3018acf7545e1112df2c11914d2f92L805).

Also a question, wouldn't make sense to have a thread pool that is reused for the whole vips live? I know it is not an easy thing to do, but spawning new threads have a cost and in my application it seems libvips is creating/destroying hundreds of thousands of them...

[jcupitt]

Quick update -- this test program:

/* compile with 
 *
 * gcc -g -Wall soak7.c `pkg-config vips --cflags --libs`
 */

#include <vips/vips.h>
#include <vips/vector.h>

int
main(int argc, char* argv[])
{
    int i;
    VipsImage* image;

    if (VIPS_INIT(argv[0]))
        vips_error_exit(NULL);

    vips_cache_set_max(0);

    image = vips_image_new_from_file(argv[1], NULL);

    for (i = 0; i < 100000; ++i) {
        VipsImage* tmp;
        double avg;
        
        printf("loop %d ..\n", i);
        vips_extract_area(image, &tmp, 0, 0, 1000, 1000, NULL);
        vips_avg(image, &avg, NULL);
        g_object_unref(tmp);
    }

    g_object_unref(image);

    vips_shutdown();

    return 0;
}

After this patch, runs like this:

iterations	kb RES
1000	40544
3000	40680
5000	40752
10000	40828
20000	41004
30000	41012
40000	41020
50000	41024
60000	41024
70000	41024
80000	41024
99999	41024

So it takes a while to stabilise (thanks to memory fragmentation) but it does seem to, eventually.

[jcupitt]

Yes, it makes a new threadpool for every evaluation loop.

I did consider a system to recycle them, but it was a lot of work to guarantee that a threadpool was no longer indirectly linked to an image. For example, threads use thread-private storage to keep certain caches, and they would all need to be flushed before reassigning a thread to a new image.

Creating and destroying threads is pretty quick, and evaluation loops generally take a long time, so the create/destroy cost is only a small percentage, unless you are processing tiny (just a few pixels) images.

[jtorresfabra]

I understand, makes sense :). Thanks for the explanation.