Remove cached datatype conversion path table entries on file close

[jhendersonHDF]

When performing datatype conversions during I/O, the library checks to see whether it can re-use a cached datatype conversion pathway by performing comparisons between the source and destination datatypes of the current operation and the source and destination datatypes associated with each cached datatype conversion pathway. For variable-length and reference datatypes, a comparison is made between the VOL object for the file associated with these datatypes, which may change as a file is closed and reopened. In workflows involving a loop that opens a file, performs I/O on an object with a variable-length or reference datatype and then closes the file, this can lead to constant memory usage growth as the library compares the file VOL objects between the datatypes as different and adds a new cached conversion pathway entry on each iteration during I/O. This is now fixed by clearing out any cached conversion pathway entries for variable-length or reference datatypes associated with a particular file when that file is closed.

[jhendersonHDF]

This PR adds a simple fix for the problem described in #1256. The main problem behind the issue is that variable-length and reference datatypes are compared according to their associated file VOL object, which may change as a file is closed and reopened. When attempting to reuse type conversion pathways in an application that has a file open -> I/O -> file close loop, the library will then compare datatypes in the current conversion operation as different from the datatypes in the cached pathway table, causing the caching table to continually grow as new entries are added until the H5T package is torn down at library termination. Further, since variable-length and reference datatypes hold a reference to their associated file's VOL object after #274, the memory for the VOL object will also be held around until the library terminates. Together, this can eventually lead to a drastic explosion of memory usage.

Several approaches for fixing this were proposed, including having a special conversion path table just for variable-length and reference datatypes, but this fix seems the most straightforward with existing library infrastructure. The downside is that it adds a scan of the cached conversion pathway table on every file close operation. If this still seems workable, I'll add a RELEASE note entry for the fix; Otherwise, we should discuss a better solution.

[jhendersonHDF]

Example program that demonstrates the issue:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include "hdf5.h"

int
main(int argc, char **argv)
{
    const char *buf[] = {"attr_value"};
    hid_t       file_id  = H5I_INVALID_HID;
    hid_t       attr_id  = H5I_INVALID_HID;
    hid_t       str_type = H5I_INVALID_HID;
    hid_t       space_id = H5I_INVALID_HID;
    int        *irbuf    = NULL;
    char       **rbuf    = NULL;

    printf("Test memory leak\n");

    file_id = H5Fcreate("file.h5", H5F_ACC_TRUNC, H5P_DEFAULT, H5P_DEFAULT);
    if (file_id == H5I_INVALID_HID) {
        printf("Couldn't create file\n");
        exit(EXIT_FAILURE);
    }

    str_type = H5Tcopy(H5T_C_S1);
    if (str_type == H5I_INVALID_HID) {
        printf("Couldn't create string type\n");
        exit(EXIT_FAILURE);
    }

    if (H5Tset_size(str_type, H5T_VARIABLE) < 0) {
        printf("Couldn't set string size\n");
        exit(EXIT_FAILURE);
    }

    space_id = H5Screate(H5S_SCALAR);
    if (space_id == H5I_INVALID_HID) {
        printf("Couldn't create dataspace\n");
        exit(EXIT_FAILURE);
    }

    attr_id = H5Acreate2(file_id, "attribute", str_type, space_id, H5P_DEFAULT, H5P_DEFAULT);
    if (attr_id == H5I_INVALID_HID) {
        printf("Couldn't create attribute\n");
        exit(EXIT_FAILURE);
    }

    if (H5Awrite(attr_id, str_type, buf) < 0) {
        printf("Couldn't write to attribute\n");
        exit(EXIT_FAILURE);
    }

    H5Aclose(attr_id);
    H5Fclose(file_id);

    irbuf = malloc(1000 * 1000 * sizeof(int));
    rbuf  = malloc(sizeof(char *));

    /* for (size_t i = 0; i < 10; i++) { */
    while (true) {
        file_id = H5Fopen("file.h5", H5F_ACC_RDONLY, H5P_DEFAULT);
        if (file_id == H5I_INVALID_HID) {
            printf("Couldn't open file\n");
            exit(EXIT_FAILURE);
        }

        attr_id = H5Aopen(file_id, "attribute", H5P_DEFAULT);
        if (attr_id == H5I_INVALID_HID) {
            printf("Couldn't open attribute\n");
            exit(EXIT_FAILURE);
        }

        if (H5Aread(attr_id, str_type, rbuf) < 0) {
            printf("Couldn't read from attribute\n");
            exit(EXIT_FAILURE);
        }

        H5Treclaim(str_type, space_id, H5P_DEFAULT, rbuf);

        H5Aclose(attr_id);
        H5Fclose(file_id);
    }

    free(irbuf);
    free(rbuf);

    H5Tclose(str_type);
    H5Sclose(space_id);

    return 0;
}

[jhendersonHDF]

We don't want to remove the vlen conversion from the soft conversions table here, so always continue on if the owned_vol_obj parameter is specified. Ideally, the logic for removing the soft conversion functions would be separate from removing the cached conversion pathway table entries, but this seems minimally invasive.

[qkoziol]

The entire for loop can be skipped if owned_vol_obj is non-NULL. (So you could add that check to the surrounding if statement, which will reduce the performance impact a little)

[jhendersonHDF]

Very true; will do

[qkoziol]

I'm clicking 'approve', but believe that if you make the two small changes I've suggested, it would be better. :-)

[qkoziol]

The entire for loop can be skipped if owned_vol_obj is non-NULL. (So you could add that check to the surrounding if statement, which will reduce the performance impact a little)

[qkoziol]

This has become increasingly unwieldy - a small "match" routine is justified?

[jhendersonHDF]

Agreed that it's become unwieldy, though is the point of a "match" routine just to move the visual ugliness out of this routine?

[qkoziol]

Mostly, and possibly to re-use

[qkoziol]

Super, thanks for updating! :-)

[tacaswell]

Even with this merged, is it worth sorting out how to manually clear this memory in h5py?

[jhendersonHDF]

Even with this merged, is it worth sorting out how to manually clear this memory in h5py?

I think I need to better understand the proxy buffering mechanism in h5py first, but I believe that this is still a problem (at least for non-string variable-length types) since I can't see anywhere else in the h5py codebase that H5Dvlen_reclaim/H5Treclaim is called after a read operation. Since this PR should fix the original issue with memory growth and since the h5py docs seem to suggest avoiding variable-length types where possible, it seems like a lower priority issue, but I think it can still be important. I quickly threw together jhendersonHDF/h5py@9004821 as a start, but it seemed a bit unwieldy to me and I was also still working on creating an h5py example for the issue.