Close objects in h5* immediate functions
#754
Conversation
Co-authored-by: Gerhard Aigner <gerhard.aigner@gmail.com>
|
Failures on Julia nightly are unrelated to this PR and are an upstream problem. |
|
Makes sense. |
|
Edit: No we're not. |
| @@ -536,6 +536,7 @@ function h5read(filename, name::AbstractString; pv...) | |||
| try | |||
| obj = getindex(fid, name; pv...) | |||
| dat = read(obj) | |||
| close(obj) | |||
There was a problem hiding this comment.
should these actually be in the finally block?
There was a problem hiding this comment.
Technically, they might need a set of nested try-catch blocks: you can't close obj if getindex failed, but then you do want to close it if read fails.
There was a problem hiding this comment.
Though allowing obj to escape the try-catch block if read fails is basically what the status quo was — GC or libhdf5's close semantics should eventually take care of the object.
There was a problem hiding this comment.
I'm not 100% confident we are following the correct close semantics in all our usages of the try--finally blocks, many of them are legacy, but I believe are mostly correct. Ideally, we could minimize their usage to only essential places, since try finally blocks are slow calls.
There was a problem hiding this comment.
Yeah, I haven't thought hard about the try-finally use yet, but IIUC they're most essential in cases like this were a file handle is being opened and closed. I agree in trying to see if more uses can be removed elsewhere, though.
There was a problem hiding this comment.
Though allowing obj to escape the try-catch block if read fails is basically what the status quo was — GC or libhdf5's close semantics should eventually take care of the object.
If read fails, close(obj) won't execute, and then we fall back to closing via libhdf5's or GC's close semantics, which were demonstrated to cause some regressions. This is an edge case and so the performance isn't critical.
There was a problem hiding this comment.
Sorry, yeah, I guess I only implied it, but that's what I meant.
The try-catch blocks make sure to close the file, but we might as well close the objects themselves deterministically rather than waiting for the GC and/or libhdf5's
H5F_CLOSE_STRONGto reap them. This should also be a net performance improvement inh5writeattreven ignoring the subtlercloseissue just due to opening the parent object only once.This was discovered as a way to improve
h5readperformance in #752 (comment) and #752 (comment).