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

Address part of #31 #34

Merged
merged 2 commits into from
Jun 25, 2019
Merged

Address part of #31 #34

merged 2 commits into from
Jun 25, 2019

Conversation

Shnatsel
Copy link
Contributor

@Shnatsel Shnatsel commented Jun 24, 2019

Fixes one occurrence of unsoundness described in #31

This change actually makes decoding 5% faster - as measured with hyperfine:

hyperfine -m 25 --warmup=3 'target/release/examples/flate -i enwiki-latest-all-titles-in-ns0.gz -o /dev/null gzip-decode'

On my machine the time goes down from 2.250s to 2.150s

Tests still pass.

@codecov-io
Copy link

codecov-io commented Jun 24, 2019

Codecov Report

Merging #34 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #34   +/-   ##
=======================================
  Coverage   91.46%   91.46%           
=======================================
  Files          14       14           
  Lines        1301     1301           
=======================================
  Hits         1190     1190           
  Misses        111      111

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 691454e...5a7bcd1. Read the comment docs.

Copy link
Owner

@sile sile left a comment

Choose a reason for hiding this comment

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

Great! Thank you so much!

@sile sile merged commit cbf1289 into sile:master Jun 25, 2019
@Shnatsel Shnatsel deleted the safe-read branch June 25, 2019 14:00
@Stargateur
Copy link
Contributor

Stargateur commented Jul 21, 2019

This could be write:

self.bit_reader
    .as_inner_mut()
    .take(len.into())
    .read_to_end(&mut self.buffer)
    .and_then(|used| {
        if used != len.into() {
            Err(io::Error::new(
                io::ErrorKind::UnexpectedEof,
                "Say something here",
            ))
        } else {
            Ok(())
        }
    })

This should be both safe and fast. I do not really follow you about your claim your PR is more fast, just with one test on your machine.

@Shnatsel
Copy link
Contributor Author

@Stargateur nice! Care to open a PR for that?

@Stargateur
Copy link
Contributor

@Shnatsel Done

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.

4 participants