Add GC-safe regions around some `ccall`s #42

kpamnany · 2023-10-25T19:54:20Z

Specifically, around LZ4_compress_fast, LZ4_compress_destSize and LZ4_decompress_safe. If these are called on large data (on the order of GBs), the calls could take multiple seconds during which GC cannot run without these GC-safe regions.

Ref: JuliaLang/julia#51574

Please note that when JuliaLang/julia#49933 lands, these changes should be subsumed by whatever that requires.

kpamnany · 2023-10-25T20:07:46Z

CI failure seems unrelated?

kuszmaul

This looks good to me. I suggested some changes to the capitalization of comments to make them consistent with the other comments in the file.

src/headers/lz4.jl

kpamnany · 2023-10-26T14:46:26Z

This needs some additional tweaks for correctness -- coming shortly.

kpamnany · 2023-10-26T17:07:11Z

This is ready to go. Thanks to @vchuravy for his input!

kpamnany · 2023-11-02T14:02:13Z

Bump. Attn: @ViralBShah or @omus or @sjkelly.

ViralBShah · 2023-11-02T14:13:07Z

@kpamnany invited you as a member of the org. Please merge and tag. Is the windows failure relevant? I think earlier PRs were not failing on windows.

kpamnany · 2023-11-02T20:46:48Z

The segfault is coming from within this ccall. I haven't touched this part of the code, but the caller of that function, here, is playing fast and loose with pointer calls. Presumably there is a missing GC.@preserve? Would love some more eyes on this... @vtjnash, please advise?

vtjnash · 2023-11-02T21:27:42Z

There does not appear to be any uses of out_buffer (aside from pointer), so there is nothing that requires the GC to bother allocating it. It is pretty risky calling pointer in that caller, as then there is nothing for GC.@preserve to preserve later in the callee.

kpamnany · 2023-11-03T00:22:54Z

There's a lot of pointer calls in there without any @preserves or any uses of the Array for which the pointer is taken. Right at the beginning of the file, here for instance.

Seems like this package needs some cleaning up.

vtjnash · 2023-11-03T01:57:53Z

Very strangely sketchy way to make unsafe_store slower and more unsafe

ViralBShah · 2023-11-03T14:00:22Z

@vtjnash What exactly should be done?

vtjnash · 2023-11-03T17:59:26Z

Someone needs to go rewrite that code without the calls to pointer

kpamnany · 2023-11-06T20:27:17Z

We have this in package cleanup on our TODO list. Unfortunately, that TODO list is very long at the moment, so I don't know how soon we'll get to this. Cc: @NHDaly

nhz2 · 2024-02-01T14:57:56Z

I have fixed a few of the pointer issues in #44 and CI is passing again. There are still more issues in the file, but this is a start. I don't have merge permissions, so someone else needs to merge this. CC: @vtjnash @kpamnany

ViralBShah · 2024-02-01T16:06:30Z

@nhz2 I have invited you to the org. Can you accept and see if you are able to merge?

nhz2 · 2024-02-01T16:36:40Z

Thanks, I am part of the JuliaIO org, but for some reason, I still see "You’re not authorized to merge this pull request." I was able to merge JuliaIO/CodecBzip2.jl#25 so there must be something special about the settings in this repo.

ViralBShah · 2024-02-01T16:58:16Z

Ah. Most of the repos had only read as the default permission for the team. I updated those - and you should be able to merge now. Glad we got this fixed!

nhz2 · 2024-02-01T17:16:59Z

I still don't have permission to merge on this repo. I see "The base branch restricts merging to authorized users. Learn more about protected branches."

ViralBShah · 2024-02-01T17:30:10Z

I don't see you as a member of JuliaIO, and it says invitation pending. In fact there were two past invitations that timed out. I don't have a direct link I can send you for accepting the invitation, as far as I can tell.

ViralBShah · 2024-02-01T17:41:17Z

Never mind - I was misspelling your id. I have made you owner.

KristofferC · 2024-02-01T17:41:45Z

I merged that PR for now. Tell me if we should make a new version as well.

ViralBShah · 2024-02-01T18:59:29Z

Might be good to get this one in and then release, but @nhz2 may be the best person to suggest.

nhz2 · 2024-02-02T00:22:02Z

I'll release a new version to get #43 in and fix PkgEval. I'm not sure what jl_gc_safe_enter does exactly, and couldn't find any docs for it, so I'm not the person to review if this PR is ready.

ViralBShah · 2024-02-02T00:35:11Z

@vtjnash Can you review and possibly fix this PR?

Specifically, around `LZ4_compress_fast`, `LZ4_compress_destSize` and `LZ4_decompress_safe`. If these are called on large data (on the order of GBs), the calls could take multiple seconds during which GC cannot run without these GC-safe regions.

codecov · 2025-03-13T17:06:11Z

Codecov Report

Attention: Patch coverage is 97.05882% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.10%. Comparing base (b3319f7) to head (4257ece).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/gcsafe_ccall.jl	96.77%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
- Coverage   98.21%   98.10%   -0.11%     
==========================================
  Files           8        9       +1     
  Lines         392      423      +31     
==========================================
+ Hits          385      415      +30     
- Misses          7        8       +1

☔ 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.

kpamnany · 2025-03-13T17:10:14Z

Sorry for leaving this PR hanging. I just updated it.

JuliaLang/julia#49933 has landed, but will only be in 1.12 so this still makes sense to do.

Okay with merging @nhz2?

vchuravy · 2025-03-13T17:47:32Z

GPUToolbox.jl is fairly lightweight and has an implementation of the macro that will be compatible with 1.12

kpamnany · 2025-03-13T20:53:30Z

Feels weird to add that dependency just for the macro though.

vchuravy · 2025-03-14T09:47:55Z

Feel free to internalize it or copy it into Compat.jl

nhz2 · 2025-03-16T01:09:38Z

I have questions about using the @gcsafe_ccall macro in other Codec packages.

Is it ok to use @gcsafe_ccall when the C function may call Julia functions for memory management, like in https://github.com/JuliaIO/CodecBzip2.jl/blob/bbccc86dc4577193cc5236516a1f4cf33cadf9b1/src/libbzip2.jl#L41-L42 ?
How should the calling convention be specified? libbzip2 needs stdcall on WIN32.

Copied from GPUToolbox.jl.

kpamnany · 2025-03-24T21:16:16Z

I've updated this PR to copy in @gsafe_ccall from GPUToolbox.jl as Kristoffer's point in the Compat.jl PR made sense to me -- better to duplicate a little code than to pull in unnecessary dependencies.

@nhz2: if you are ccalling a function that calls back into Julia, that is not GC-safe.

@vchuravy: can I get a little help with the CI failures on Julia 1.6 please? I do not know how @inline has changed. Also, I do not know the answer to @nhz2's second question.

kpamnany · 2025-03-28T12:37:43Z

Turns out that support for using @inline as a call-site annotation was only added in 1.8. And I don't think I can do anything more about code coverage.

So this is good to go, AFAICT. Would love to have someone take a quick look before merging.

KristofferC · 2025-03-28T12:41:00Z

Would it make sense to add 1.12-nightly and nightly to the CI matrix since there are some different branches that will be hit for those?

kpamnany · 2025-03-28T14:14:17Z

Apart from that, looks okay to you Kristoffer?

kuszmaul approved these changes Oct 26, 2023

View reviewed changes

kpamnany force-pushed the kp/gc-safe-some-cccalls branch 2 times, most recently from adf1dc8 to 0ecf4f8 Compare October 26, 2023 16:49

kpamnany force-pushed the kp/gc-safe-some-cccalls branch from 0ecf4f8 to db79d9c Compare November 3, 2023 00:19

Add GC-safe regions around some ccalls

e31fa66

Specifically, around `LZ4_compress_fast`, `LZ4_compress_destSize` and `LZ4_decompress_safe`. If these are called on large data (on the order of GBs), the calls could take multiple seconds during which GC cannot run without these GC-safe regions.

kpamnany force-pushed the kp/gc-safe-some-cccalls branch from abcacee to e31fa66 Compare March 13, 2025 17:04

kpamnany added 2 commits March 24, 2025 17:06

Add @gcsafe_ccall

104adaa

Copied from GPUToolbox.jl.

Use @gcsafe_ccall

a9699a1

kpamnany added 2 commits March 26, 2025 15:00

@inline block was only added in 1.8

f6a5e08

Add test for @gcsafe_ccall

d1e5e08

kpamnany force-pushed the kp/gc-safe-some-cccalls branch from 0ce15b6 to d1e5e08 Compare March 27, 2025 22:40

Add 'nightly' to CI matrix

4257ece

KristofferC merged commit 1a5932d into JuliaIO:master Mar 28, 2025
10 of 12 checks passed

Add GC-safe regions around some ccalls #42

Add GC-safe regions around some ccalls #42

Uh oh!

Conversation

kpamnany commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kpamnany commented Oct 25, 2023

Uh oh!

kuszmaul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kpamnany commented Oct 26, 2023

Uh oh!

kpamnany commented Oct 26, 2023

Uh oh!

kpamnany commented Nov 2, 2023

Uh oh!

ViralBShah commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kpamnany commented Nov 2, 2023

Uh oh!

vtjnash commented Nov 2, 2023

Uh oh!

kpamnany commented Nov 3, 2023

Uh oh!

vtjnash commented Nov 3, 2023

Uh oh!

ViralBShah commented Nov 3, 2023

Uh oh!

vtjnash commented Nov 3, 2023

Uh oh!

kpamnany commented Nov 6, 2023

Uh oh!

nhz2 commented Feb 1, 2024

Uh oh!

ViralBShah commented Feb 1, 2024

Uh oh!

nhz2 commented Feb 1, 2024

Uh oh!

ViralBShah commented Feb 1, 2024

Uh oh!

nhz2 commented Feb 1, 2024

Uh oh!

ViralBShah commented Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ViralBShah commented Feb 1, 2024

Uh oh!

KristofferC commented Feb 1, 2024

Uh oh!

ViralBShah commented Feb 1, 2024

Uh oh!

nhz2 commented Feb 2, 2024

Uh oh!

ViralBShah commented Feb 2, 2024

Uh oh!

codecov bot commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kpamnany commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vchuravy commented Mar 13, 2025

Uh oh!

kpamnany commented Mar 13, 2025

Uh oh!

vchuravy commented Mar 14, 2025

Uh oh!

nhz2 commented Mar 16, 2025

Uh oh!

kpamnany commented Mar 24, 2025

Add GC-safe regions around some `ccall`s #42

Add GC-safe regions around some `ccall`s #42

kpamnany commented Oct 25, 2023 •

edited

Loading

ViralBShah commented Nov 2, 2023 •

edited

Loading

ViralBShah commented Feb 1, 2024 •

edited

Loading

codecov bot commented Mar 13, 2025 •

edited

Loading

kpamnany commented Mar 13, 2025 •

edited

Loading

KristofferC commented Mar 28, 2025 •

edited

Loading