Skip to content

Remove initialize and make finalize optional - #71

Closed
nhz2 wants to merge 3 commits into
masterfrom
nz/remove-initialize-and-finalize
Closed

Remove initialize and make finalize optional#71
nhz2 wants to merge 3 commits into
masterfrom
nz/remove-initialize-and-finalize

Conversation

@nhz2

@nhz2nhz2 commented Sep 15, 2024

Copy link
Copy Markdown
Member

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

@codecov

codecovBot commented Sep 15, 2024

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.83333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 60.64%. Comparing base (e7edfed) to head (fdb6ec9).
⚠️ Report is 4 commits behind head on master.

Files with missing linesPatch %Lines
src/compression.jl91.66%1 Missing ⚠️
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 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

@nhz2

This comment was marked as resolved.

Comment threadsrc/libzstd.jl
C_NULL,
)
ptr = ccall(
(:ZSTD_createCStream_advanced, libzstd),

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ZSTD_createCStream_advanced is part of the experimental API though according to facebook/zstd#1734 these functions should not change much in the future.

I am using it because the regular ZSTD_createCStream leads to Julia not running the finalizer before being killed by OOM.

@nhz2
nhz2 marked this pull request as ready for review September 15, 2024 19:48
@nhz2
nhz2 requested a review from mkittiSeptember 15, 2024 19:48
@mkitti

Copy link
Copy Markdown
Member

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

Comment threadsrc/libzstd.jl Outdated
@nhz2

nhz2 commented Sep 15, 2024

Copy link
Copy Markdown
MemberAuthor

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
nhz2 marked this pull request as draft September 15, 2024 20:41
@nhz2nhz2 changed the title Remove initialize and finalizeRemove initialize and make finalize optionalSep 15, 2024
@nhz2
nhz2 marked this pull request as ready for review September 15, 2024 21:10
Comment threadProject.toml
TranscodingStreams = "0.9, 0.10, 0.11"
Zstd_jll = "1.5.5"
julia = "1.3"
julia = "1.7"

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

atomics require 1.7

@nhz2nhz2 mentioned this pull request Sep 15, 2024
@nhz2
nhz2 marked this pull request as draft September 16, 2024 16:11
@nhz2nhz2 mentioned this pull request Sep 16, 2024
@nhz2

nhz2 commented Sep 8, 2025

Copy link
Copy Markdown
MemberAuthor

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.

@nhz2nhz2 closed this Sep 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reusing a compressor

2 participants

@nhz2@mkitti