CodecZstd.jl icon indicating copy to clipboard operation
CodecZstd.jl copied to clipboard

Remove `initialize` and make `finalize` optional

Open nhz2 opened this issue 1 year ago • 4 comments

Fixes #70 by initializing on construction and finalizing in a finalizer.

nhz2 avatar Sep 15 '24 17:09 nhz2

Codecov Report

:x: Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review. :white_check_mark: Project coverage is 60.64%. Comparing base (e7edfed) to head (fdb6ec9). :warning: Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/compression.jl 91.66% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #71      +/-   ##
==========================================
- Coverage   61.55%   60.64%   -0.92%     
==========================================
  Files           5        5              
  Lines         372      371       -1     
==========================================
- Hits          229      225       -4     
- Misses        143      146       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Sep 15 '24 17:09 codecov[bot]

Unfortunatly this PR creates OOM issues:

julia> using CodecZstd

julia> function foo()
       for i in 1:500000
       transcode(ZstdCompressor,zeros(UInt8,16))
       end
       end
foo (generic function with 1 method)

julia> foo()
Killed

nhz2 avatar Sep 15 '24 18:09 nhz2

We should not rely on the GC to finalize external resources.

mkitti avatar Sep 15 '24 19:09 mkitti

We should not rely on the GC to finalize external resources.

Should I try and support both explicit-free when we know the Codec cannot escape and automatic-free for more complex situations?

nhz2 avatar Sep 15 '24 20:09 nhz2

I'm going to close this PR because I don't like that it uses an experimental API that may break in the future. With Zstd_jll being a standard library, this means a future version of Julia could be incompatible with this PR.

Also, with some options, the zstd compression and decompression context can use multiple GB's of memory, so users may need more control over when that gets allocated and freed.

nhz2 avatar Sep 08 '25 21:09 nhz2