Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New compression functions #209

Merged
merged 6 commits into from
Feb 27, 2024
Merged

New compression functions #209

merged 6 commits into from
Feb 27, 2024

Conversation

LGFae
Copy link
Owner

@LGFae LGFae commented Feb 16, 2024

This overhauls the current api for compressing animated frames.

The new api is more explicit about errors, does better checking for preconditions, and is faster.

We've dropped lzzzz in favor of just linking the lz4 system library directly. lz4 was already listed as one of our dependencies, so this should be fine.

We've also included hand-written SIMD implementations to further speed up both compression and decompression. Note that LZ4 takes the most amount of time when it comes to compression and decompression, so, even though this is nice, it won't make a world of difference.

@Akida31
Copy link
Contributor

Akida31 commented Feb 17, 2024

Hey, I saw this PR, got excited about the potential (didn't profile this yet) speed-up, skimmed the changes and saw that you used some unsafe code.
For me, miri throws an error that there is undefined behaviour (I ran cargo +nightly miri test in utils):
miri says "Undefined Behavior: memory access failed: alloc13209688 has size 1510, so pointer to 4 bytes starting at offset 1507 is out-of-bounds"

Therefore I tried to verify the error and it looks like there is indeed an out of bounds read in compression::decomp::unpack_bytes.

additional check to reproduce
diff --git a/utils/src/compression/decomp/mod.rs b/utils/src/compression/decomp/mod.rs
index f7bfd9c..c7443c2 100644
--- a/utils/src/compression/decomp/mod.rs
+++ b/utils/src/compression/decomp/mod.rs
@@ -38,6 +38,12 @@ pub(super) fn unpack_bytes(buf: &mut [u8], diff: &[u8]) {
         diff_idx += 1;
 
         for _ in 0..to_cpy {
+            if diff_idx + 4 > len {
+                panic!(
+                    "index out of range: diff_idx={diff_idx} diff_idx+4={} len={len}",
+                    diff_idx + 4
+                );
+            }
             unsafe { std::ptr::copy_nonoverlapping(diff.add(diff_idx), buf.add(pix_idx * 4), 4) }
             diff_idx += 3;
             pix_idx += 1;

I only skimmed the code and you certainly understand the code better than me, but I wanted to point this out and maybe you know if this is a problem or just miri (and me) being noisy :)

@LGFae
Copy link
Owner Author

LGFae commented Feb 17, 2024

Thanks a lot for pointing that out, @Akida31.

Interestingly, this is a problem that also existed in the previous implementation! It just hadn't blown up yet.

I've gone ahead and added some debug_assertions to make sure this shows up in future tests. I might add even more assertions before the final merge.

@Akida31
Copy link
Contributor

Akida31 commented Feb 27, 2024

I ran miri after that and it looks like the errors are gone now.
Sadly miri takes really long to run all tests, on my computer like 30 minutes. So I think it is not really feasible to run miri in CI right now. But it could be worth trying to reduce the number iterations for miri so that this will be possible.
Some other thoughts to this PR:

  • If somebody has the energy and time I think it would be really good to review all unsafe code and write safety comments and document the assertions (e.g. utils/src/compression/mod.rs:77) which are necessary for soundness and may not be removed.
  • I think there are some functions unnecessarily public which can be private (for example in utils/src/compression/comp/mod.rs)
  • I think the simd checking macro/ functions could use OnceCell and not a static mut removing some unsafe code.

But these are just my thoughts and not blocking concerns for this PR, feel free to decide which of these points are good and which to ignore.
All in all I think the speedup will be nice :)

@LGFae
Copy link
Owner Author

LGFae commented Feb 27, 2024

I think your 1st and 2nd point are very valid, and I've gone ahead and documented most of the usage of unsafe and their soundness requirements.

Regarding your 3rd point, I just don't like OnceCell very much. It's a silly personal preference that might change in the future, but I'll leave it like this for now.

Thanks a lot for your guidance and reviews!

@LGFae LGFae merged commit f77d1e0 into main Feb 27, 2024
9 checks passed
@LGFae LGFae deleted the new-compression-functions branch February 27, 2024 16:05
Sign up for free to 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.

2 participants