Add bitshuffle #986

jamesrhester · 2022-07-07T00:57:43Z

This adds the bitshuffle filter for HDF5. Closes #981.

mkitti · 2022-07-07T03:09:35Z

Great. Could you add some tests?
https://github.com/JuliaIO/HDF5.jl/blob/master/test/filter.jl

jamesrhester · 2022-07-07T03:43:55Z

Will do.

mkitti · 2022-07-07T05:09:16Z

I think you are missing an implementation of bshuf_h5_set_local function.
https://github.com/kiyo-masui/bitshuffle/blob/master/src/bshuf_h5filter.c#L29

jamesrhester · 2022-07-07T05:11:09Z

Yes, working on it now. Having to write tests meant I discovered that I'd ignored the writing part of the filter...

mkitti · 2022-07-07T05:19:53Z

See

HDF5.jl/filters/H5Zblosc/src/H5Zblosc.jl

Line 19 in ccdb3e9

function blosc_set_local(dcpl::API.hid_t, htype::API.hid_t, space::API.hid_t)

if you need an example of that.

Also, we could just compile the C code for the plugin, probably as a separate Yggrassil recipe. We could then just use the H5PL interface to tell HDF5 where the plugin is.

jamesrhester · 2022-07-07T11:31:00Z

It turns out that I put the lower compat limit of bitshuffle_jll as 1.6, have to wait for that to be fixed.

jamesrhester · 2022-07-07T12:03:43Z

OK, I'm informed (see JuliaPackaging/Yggdrasil#5111) that the lower compatibility limit for bitshuffle_jll should be 1.6. So I don't know if I should be trying to pass the Julia 1.3 checks?

mkitti · 2022-07-07T13:13:30Z

These tests should only run with 1.6 then. Just use a @static if VERSION >= v"1.6" to qualify the tests and the package import.

filters/H5Zbitshuffle/src/H5Zbitshuffle.jl

jamesrhester · 2022-07-08T02:56:12Z

I have used @static as suggested, not sure how to manage the import resolution fail for the 1.3 tests.

filters/H5Zbitshuffle/src/H5Zbitshuffle.jl

Co-authored-by: Mark Kittisopikul <[email protected]>

filters/H5Zbitshuffle/src/H5Zbitshuffle.jl

test/filter.jl

test/runtests.jl

Co-authored-by: Mark Kittisopikul <[email protected]>

mkitti · 2022-07-08T06:31:58Z

I'm happy to merge this if you are. Thanks again.

jamesrhester · 2022-07-08T07:06:19Z

Yes, I'm happy to merge. Thanks for your review and help.

mkitti · 2022-07-08T15:40:58Z

I'm pausing for a few days to give others the chance to review. I'm also going to do some further testing with plugins from other sources and via different APIs.

mkitti · 2022-07-11T07:56:39Z

Can the license correctly be categorized as a MIT license?

jamesrhester · 2022-07-11T08:55:16Z

Yes, it was meant to be a MIT license copied from the original bitshuffle distribution. Thanks for merging this.

mkitti · 2022-07-12T22:36:01Z

@jamesrhester , I'm seeing a sporadic CI failure on 64-bit Windows:
https://github.com/JuliaIO/HDF5.jl/runs/7292646756?check_suite_focus=true

We may need to review

jamesrhester · 2022-07-13T02:20:37Z

Looks like nightly is OK, but Julia 1.7 has a problem. I'll see if I can spin up a Windows 64 VM here and run the test repeatedly.

jamesrhester · 2022-07-13T03:01:38Z

I'm suspecting it could be because of a lack of @GC.preserve at the end of the filter function with the unsafe_load and unsafe_store! calls. The corresponding H5Zblosc filter doesn't have these, however, so once I've got a test VM running I'll try and duplicate the error and then see if the @GC.preserve fixes it.

mkitti · 2022-07-13T05:27:56Z

The original code allocates 11 Cuints for values, but you only allocate 8 (similar to tmp_values) in bitshuffle_set_local. You also don't seem to move the values to a higher position. I think you may be overwriting values stored in bs_values by API.h5p_get_filter_by_id. Perhaps I am missing something?

https://github.com/kiyo-masui/bitshuffle/blob/fdfcd404ac8dcb828857a90c559d36d8ac4c2968/src/bshuf_h5filter.c#L47-L50

You also do not bump nelements by 3.

mkitti · 2022-07-13T06:30:20Z

I see now that you added three extra values to the struct similar to how we implemented the Blosc filter.

mkitti · 2022-07-13T06:36:02Z

Hmm, you never initialize bs_values to 0.

jamesrhester · 2022-07-13T06:47:42Z

The way this works, as I understand it, is that the set_local routine provides an opportunity to fill and check values in the BitshuffleFilter struct, which is what bs_values is filled with after H5API.h5p_get_filter_by_id. It would also be OK to completely skip this step and set everything at BitshuffleFilter initialisation, except that the element size isn't known at the time that the filter is constructed. So in the Julia version I have set almost every value at initialisation, except for element size, but repeat the setting of MAJOR, MINOR values.
Still sorting out access to a Windows VM.

mkitti

The other things I noticed were:

The values are zero initialized.
We probably should integer division. / in Julia will produce a Float64.

mkitti · 2022-07-13T05:39:09Z

filters/H5Zbitshuffle/src/H5Zbitshuffle.jl

+                nbytes_uncomp = ccall((:bshuf_read_uint64_BE,libbitshuffle),Cuint,(Ptr{Cvoid},),in_buf)
+                # Next 4 bytes are the block size
+
+                block_size = ccall((:bshuf_read_uint32_BE,libbitshuffle),Cuint,(Ptr{Cvoid},),in_buf+8)/elem_size


I think we should ÷ here rather than /. block_size should be an integer.

Makes sense.

mkitti · 2022-07-13T05:40:02Z

filters/H5Zbitshuffle/src/H5Zbitshuffle.jl

+                nbytes_uncomp = nbytes
+                if compress_flag == BSHUF_H5_COMPRESS_LZ4
+                    buf_size_out = ccall((:bshuf_compress_lz4_bound,libbitshuffle),Cuint,(Cuint,Cuint,Cuint),
+                                         nbytes_uncomp/elem_size,elem_size,block_size) + 12


Use ÷ here rather than /, though Julia might take care of translating this back for us here.

mkitti · 2022-07-13T05:40:11Z

filters/H5Zbitshuffle/src/H5Zbitshuffle.jl

+                                         nbytes_uncomp/elem_size,elem_size,block_size) + 12
+                elseif compress_flag == BSHUF_H5_COMPRESS_ZSTD
+                    buf_size_out = ccall((:bshuf_compress_zstd_bound,libbitshuffle),Cuint,(Cuint,Cuint,Cuint),
+                                         nbytes_uncomp/elem_size,elem_size,block_size)+12


Use ÷ here rather than /, though Julia might take care of translating this back for us here.

mkitti · 2022-07-13T05:41:49Z

filters/H5Zbitshuffle/src/H5Zbitshuffle.jl

+            error("bitshuffle_h5plugin: Uncompressed size $nbytes_uncomp is not a multiple of $elem_size")
+        end
+
+        size = nbytes_uncomp/elem_size


Use ÷ here rather than /

mkitti · 2022-07-13T07:45:03Z

One issue I encountered that relates to GC.@preserve and decompression is detailed here:
JuliaLang/julia#43402

This was due a specific change on the development branch of Julia though.

What happens if bs_values is getting freed after bitshuffle_set_local but before H5Z_filter_bitshuffle finishes?

I have a Windows box, and so far I am not able to reproduce the error reliably.

mkitti · 2022-07-13T07:57:22Z

I'm going to try to focus testing here:
#988

jamesrhester · 2022-07-13T08:11:34Z

One issue I encountered that relates to GC.@preserve and decompression is detailed here: JuliaLang/julia#43402

This was due a specific change on the development branch of Julia though.

I read through that but couldn't see how it applies here. Weird bug though.

What happens if bs_values is getting freed after bitshuffle_set_local but before H5Z_filter_bitshuffle finishes?

Well, as bs_values is of type Ptr{Cuint} it should not get freed by Julia if I understand the whole Ptr, Ref thing.

mkitti · 2022-07-13T08:16:40Z

Well, as bs_values is of type Ptr{Cuint} it should not get freed by Julia if I understand the whole Ptr, Ref thing.

The problem is that you allocate it via bs_values = Vector{Cuint}(undef,8). When it gets passed to ccall, the pointer behind the Vector is passed to HDF5. After bitshuffle_set_local completes, Julia thinks it can free the pointer behind bs_values.

The question is if H5Pmodify_filter just hangs on to the pointer or if it makes a copy.

mkitti · 2022-07-13T08:44:14Z

H5Pmodify_filter does seem to copy the data:
https://github.com/HDFGroup/hdf5/blob/c064d3481b582653c1e0d0043a17527fd73e8c4d/src/H5Z.c#L1095-L1096

mkitti · 2022-07-13T08:55:06Z

Aha, I found something.

The C function signature is:

int64_t bshuf_decompress_lz4(const void* in, void* out, const size_t size,
        const size_t elem_size, size_t block_size) {

The ccall however is

                    err = ccall((:bshuf_decompress_lz4,libbitshuffle),Cint,
                                (Ptr{Cvoid},Ptr{Cvoid},Cuint,Cuint,Cuint),
                                in_buf,out_buf,size,elem_size,block_size)

This should be

                    err = ccall((:bshuf_decompress_lz4,libbitshuffle),Int64,
                                (Ptr{Cvoid},Ptr{Cvoid},Csize_t,Csize_t,Csize_t),
                                in_buf,out_buf,size,elem_size,block_size)

julia> Csize_t
UInt64

julia> Cuint
UInt32

James.Hester added 3 commits July 7, 2022 10:35

Initial commit of bitshuffle filter.

14a6b5f

Add README, License, improve comments.

f9dd5df

Code layout improvement

ccbc021

James.Hester added 3 commits July 7, 2022 16:58

Add writing functionality, add tests

ffb0e79

Fix bitshuffle writing issues.

65e8e92

Minimum Julia for bitshuffle set to 1.6

2ed33f5

mkitti reviewed Jul 7, 2022

View reviewed changes

filters/H5Zbitshuffle/src/H5Zbitshuffle.jl Show resolved Hide resolved

Version-specific import of bitshuffle filter when testing

68e5b36

mkitti reviewed Jul 8, 2022

View reviewed changes

filters/H5Zbitshuffle/src/H5Zbitshuffle.jl Outdated Show resolved Hide resolved

Improve formatting

184e7a4

Co-authored-by: Mark Kittisopikul <[email protected]>

mkitti reviewed Jul 8, 2022

View reviewed changes

filters/H5Zbitshuffle/src/H5Zbitshuffle.jl Outdated Show resolved Hide resolved

mkitti reviewed Jul 8, 2022

View reviewed changes

test/filter.jl Outdated Show resolved Hide resolved

mkitti reviewed Jul 8, 2022

View reviewed changes

test/runtests.jl Outdated Show resolved Hide resolved

jamesrhester and others added 2 commits July 8, 2022 15:34

Add conditional package load

ae4cf3c

Co-authored-by: Mark Kittisopikul <[email protected]>

Code style improvements.

2bb3498

mkitti approved these changes Jul 8, 2022

View reviewed changes

mkitti requested a review from musm July 8, 2022 07:08

mkitti merged commit 3a24c8e into JuliaIO:master Jul 11, 2022

mkitti reviewed Jul 13, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bitshuffle #986

Add bitshuffle #986

jamesrhester commented Jul 7, 2022

mkitti commented Jul 7, 2022

jamesrhester commented Jul 7, 2022

mkitti commented Jul 7, 2022

jamesrhester commented Jul 7, 2022

mkitti commented Jul 7, 2022

jamesrhester commented Jul 7, 2022

jamesrhester commented Jul 7, 2022

mkitti commented Jul 7, 2022

jamesrhester commented Jul 8, 2022 •

edited

Loading

mkitti commented Jul 8, 2022

jamesrhester commented Jul 8, 2022

mkitti commented Jul 8, 2022

mkitti commented Jul 11, 2022

jamesrhester commented Jul 11, 2022

mkitti commented Jul 12, 2022

jamesrhester commented Jul 13, 2022

jamesrhester commented Jul 13, 2022

mkitti commented Jul 13, 2022 •

edited

Loading

mkitti commented Jul 13, 2022

mkitti commented Jul 13, 2022

jamesrhester commented Jul 13, 2022

mkitti left a comment

mkitti Jul 13, 2022

jamesrhester Jul 13, 2022

mkitti Jul 13, 2022

mkitti Jul 13, 2022

mkitti Jul 13, 2022

mkitti commented Jul 13, 2022

mkitti commented Jul 13, 2022

jamesrhester commented Jul 13, 2022

mkitti commented Jul 13, 2022

mkitti commented Jul 13, 2022

mkitti commented Jul 13, 2022 •

edited

Loading

Add bitshuffle #986

Add bitshuffle #986

Conversation

jamesrhester commented Jul 7, 2022

mkitti commented Jul 7, 2022

jamesrhester commented Jul 7, 2022

mkitti commented Jul 7, 2022

jamesrhester commented Jul 7, 2022

mkitti commented Jul 7, 2022

jamesrhester commented Jul 7, 2022

jamesrhester commented Jul 7, 2022

mkitti commented Jul 7, 2022

jamesrhester commented Jul 8, 2022 • edited Loading

mkitti commented Jul 8, 2022

jamesrhester commented Jul 8, 2022

mkitti commented Jul 8, 2022

mkitti commented Jul 11, 2022

jamesrhester commented Jul 11, 2022

mkitti commented Jul 12, 2022

jamesrhester commented Jul 13, 2022

jamesrhester commented Jul 13, 2022

mkitti commented Jul 13, 2022 • edited Loading

mkitti commented Jul 13, 2022

mkitti commented Jul 13, 2022

jamesrhester commented Jul 13, 2022

mkitti left a comment

Choose a reason for hiding this comment

mkitti Jul 13, 2022

Choose a reason for hiding this comment

jamesrhester Jul 13, 2022

Choose a reason for hiding this comment

mkitti Jul 13, 2022

Choose a reason for hiding this comment

mkitti Jul 13, 2022

Choose a reason for hiding this comment

mkitti Jul 13, 2022

Choose a reason for hiding this comment

mkitti commented Jul 13, 2022

mkitti commented Jul 13, 2022

jamesrhester commented Jul 13, 2022

mkitti commented Jul 13, 2022

mkitti commented Jul 13, 2022

mkitti commented Jul 13, 2022 • edited Loading

jamesrhester commented Jul 8, 2022 •

edited

Loading

mkitti commented Jul 13, 2022 •

edited

Loading

mkitti commented Jul 13, 2022 •

edited

Loading