Skip to content

Protection from attempting to do SIMD rans encoding on small data#137

Merged
daviesrob merged 2 commits into
samtools:masterfrom
jkbonfield:rans_simd_protect
Jul 1, 2025
Merged

Protection from attempting to do SIMD rans encoding on small data#137
daviesrob merged 2 commits into
samtools:masterfrom
jkbonfield:rans_simd_protect

Conversation

@jkbonfield
Copy link
Copy Markdown
Collaborator

We can't just change do_simd as we may then have a combination of SIMD (pack or rle-meta) and non-SIMD (rle-meta or rle-data). So instead we remove the ability to do SIMD and call again.

This only happens when we have large blocks of data that enable SIMD mode which then turn out to be heavily compressible after PACK or RLE to yield a small sub-stream which isn't SIMD compressible.

Comment thread htscodecs/rANS_static4x16pr.c Outdated
Comment on lines +1394 to +1398
free(packed);
return rans_compress_to_4x16(orig_in, orig_in_size,
out, out_size,
orig_order & ~(RANS_ORDER_X32 |
RANS_ORDER_SIMD_AUTO));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This works, but wouldn't it be simpler to just drop X32 mode and carry on:

                do_simd = 0;
                out[0] &= ~RANS_ORDER_X32;

so that you avoid having to re-do all the RLE / packing work? (And the same below...)

@jkbonfield
Copy link
Copy Markdown
Collaborator Author

jkbonfield commented Jun 30, 2025

Also added some test data to demonstrate the SIMD issue.

We can't just change do_simd as we may then have a combination of SIMD
(pack or rle-meta) and non-SIMD (rle-meta or rle-data).  So instead we
remove the ability to do SIMD.

This only happens when we have large blocks of data that enable SIMD
mode which then turn out to be heavily compressible after PACK or RLE
to yield a small sub-stream which isn't SIMD compressible.
@daviesrob daviesrob force-pushed the rans_simd_protect branch from f77342d to 448b1b9 Compare July 1, 2025 15:26
@daviesrob
Copy link
Copy Markdown
Member

Fixed a minor white-space change...

@daviesrob daviesrob merged commit 448b1b9 into samtools:master Jul 1, 2025
6 checks passed
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