Skip to content

Conversation

@folkertdev
Copy link
Member

especially for smaller files this is an improvement

Benchmark 2 (180 runs): target/relwithdebinfo/examples/decompress rs tests/input/bzip2-testfiles/dotnetzip/dancing-color.ps.bz2
  measurement          mean ± σ            min … max           outliers         delta
  wall_time          27.9ms ±  352us    27.2ms … 31.0ms          2 ( 1%)        ⚡-  2.3% ±  0.3%
  peak_rss           7.54MB ± 67.0KB    7.34MB … 7.60MB          0 ( 0%)          +  0.0% ±  0.2%
  cpu_cycles          113M  ± 1.08M      112M  …  125M          11 ( 6%)        ⚡-  2.6% ±  0.3%
  instructions        205M  ±  289       205M  …  205M           0 ( 0%)        ⚡-  3.0% ±  0.0%
  cache_references   2.49M  ± 17.8K     2.47M  … 2.61M          35 (19%)          +  0.9% ±  0.2%
  cache_misses        545K  ± 2.69K      538K  …  561K           4 ( 2%)        💩+  4.0% ±  0.2%
  branch_misses       948K  ± 1.99K      942K  …  957K           7 ( 4%)        ⚡- 15.5% ±  0.1%

(CI benchmarks just show noise...)

@codecov
Copy link

codecov bot commented Jan 24, 2025

Codecov Report

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

Files with missing lines Patch % Lines
libbz2-rs-sys/src/decompress.rs 95.83% 1 Missing ⚠️
Files with missing lines Coverage Δ
libbz2-rs-sys/src/decompress.rs 94.24% <95.83%> (+0.02%) ⬆️

... and 2 files with indirect coverage changes

for t in 0..nGroups {
let mut curr = GET_BITS!(strm, s, 5);
for i in 0..alphaSize {
loop {
Copy link
Member Author

Choose a reason for hiding this comment

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

it should be possible to bound this loop (I think it's 20 bits of code at most, so 40bits total?). But it's hard to be confident

Copy link
Member Author

Choose a reason for hiding this comment

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

I fuzzed this for many hours using this size of 20. So it holds at least for files encoded using the current version (which uses 17 bits at most, while earlier bzip2 versions used up to 20 bits).

@folkertdev folkertdev force-pushed the non-stream-coding-tables branch from 1c3ddbd to 920297e Compare January 28, 2025 16:13

s.len[usize::from(t)][usize::from(i)] = curr as u8;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What code is this block based on?

Copy link
Member Author

Choose a reason for hiding this comment

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

this bit of C code

libbzip2-rs/decompress.c

Lines 322 to 335 in d5d181b

/*--- Now the coding tables ---*/
for (t = 0; t < nGroups; t++) {
GET_BITS(BZ_X_CODING_1, curr, 5);
for (i = 0; i < alphaSize; i++) {
while (True) {
if (curr < 1 || curr > 20) RETURN(BZ_DATA_ERROR);
GET_BIT(BZ_X_CODING_2, uc);
if (uc == 0) break;
GET_BIT(BZ_X_CODING_3, uc);
if (uc == 0) curr++; else curr--;
}
s->len[t][i] = curr;
}
}

it's all scattered around in our implementation here, but if we assume sufficient input, we can basically copy the C code.

@folkertdev folkertdev merged commit a417f37 into main Feb 6, 2025
21 checks passed
@folkertdev folkertdev deleted the non-stream-coding-tables branch February 6, 2025 14:57
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.

3 participants