Add GC-safe regions around some ccalls

[kpamnany]

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]

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.

[kpamnany]

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

[kpamnany]

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

[kpamnany]

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

[ViralBShah]

@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]

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]

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]

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]

Very strangely sketchy way to make unsafe_store slower and more unsafe

[ViralBShah]

@vtjnash What exactly should be done?

[vtjnash]

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

[kpamnany]

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]

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]

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

[nhz2]

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]

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]

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]

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]

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

[KristofferC]

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

[ViralBShah]

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

[nhz2]

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]

@vtjnash Can you review and possibly fix this PR?