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

Don't panic on drop #100

Merged
merged 4 commits into from
Apr 7, 2023
Merged

Conversation

CosmicHorrorDev
Copy link
Contributor

This makes the Drop impl for AutoFinishEncoder<_> infallible. Panicking on drop is a very easy way to cause a program to abort since drop impls run while unwinding a panic which can cause a panic while already panicking

I tried making Encoder automatically call .try_finish() on drop, so that AutoFinishEncoder<_> wouldn't be needed in the first place, but my implementation probably regressed performance too much for your liking even though running finalization on drop is common (see zip, xz2, etc). I'll keep toying around with it to see if I can figure out the cause of the only severe regression (FrameCompress/lz4_flex_rust_(indep|linked)/66675). It goes with the common technique of wrapping the writer in an Option<_>, so that it can be .taken by methods like .into_inner() while still providing a drop impl

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #100 (10ecc5d) into main (be2ea76) will decrease coverage by 0.87%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
- Coverage   89.39%   88.53%   -0.87%     
==========================================
  Files          11       11              
  Lines        2179     2189      +10     
==========================================
- Hits         1948     1938      -10     
- Misses        231      251      +20     
Impacted Files Coverage Δ
src/frame/compress.rs 78.42% <0.00%> (+0.96%) ⬆️
src/frame/mod.rs 53.33% <ø> (ø)

... and 2 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@CosmicHorrorDev
Copy link
Contributor Author

Here's the other branch I'm talking about above https://github.com/CosmicHorrorDev/lz4_flex/tree/remove-AutoFinishEncoder

And here are the benchmark results for FrameCompress running on that branch. Still need to dig into things more

FrameCompress/lz4_flex_rust_indep/725
                        time:   [1.5810 µs 1.5863 µs 1.5941 µs]
                        thrpt:  [433.74 MiB/s 435.87 MiB/s 437.33 MiB/s]
                 change:
                        time:   [-2.2717% -1.6749% -0.8107%] (p = 0.00 < 0.05)
                        thrpt:  [+0.8173% +1.7034% +2.3245%]
                        Change within noise threshold.
FrameCompress/lz4_flex_rust_linked/725
                        time:   [1.6072 µs 1.6275 µs 1.6573 µs]
                        thrpt:  [417.19 MiB/s 424.82 MiB/s 430.21 MiB/s]
                 change:
                        time:   [-1.8719% -1.1239% -0.3236%] (p = 0.00 < 0.05)
                        thrpt:  [+0.3247% +1.1367% +1.9076%]
                        Change within noise threshold.
FrameCompress/lz4_flex_rust_indep/34308
                        time:   [72.236 µs 72.384 µs 72.555 µs]
                        thrpt:  [450.95 MiB/s 452.02 MiB/s 452.94 MiB/s]
                 change:
                        time:   [+6.5282% +6.9285% +7.3466%] (p = 0.00 < 0.05)
                        thrpt:  [-6.8438% -6.4795% -6.1282%]
                        Performance has regressed.
FrameCompress/lz4_flex_rust_linked/34308
                        time:   [70.437 µs 70.609 µs 70.798 µs]
                        thrpt:  [462.14 MiB/s 463.38 MiB/s 464.51 MiB/s]
                 change:
                        time:   [+2.8315% +3.3423% +3.7652%] (p = 0.00 < 0.05)
                        thrpt:  [-3.6286% -3.2342% -2.7535%]
                        Performance has regressed.
FrameCompress/lz4_flex_rust_indep/64723
                        time:   [147.66 µs 147.90 µs 148.16 µs]
                        thrpt:  [416.60 MiB/s 417.35 MiB/s 418.01 MiB/s]
                 change:
                        time:   [+1.7641% +2.1645% +2.6131%] (p = 0.00 < 0.05)
                        thrpt:  [-2.5466% -2.1187% -1.7336%]
                        Performance has regressed.
FrameCompress/lz4_flex_rust_linked/64723
                        time:   [149.24 µs 149.58 µs 149.91 µs]
                        thrpt:  [411.74 MiB/s 412.65 MiB/s 413.60 MiB/s]
                 change:
                        time:   [+1.1331% +1.7471% +2.2979%] (p = 0.00 < 0.05)
                        thrpt:  [-2.2463% -1.7171% -1.1204%]
                        Performance has regressed.
FrameCompress/lz4_flex_rust_indep/66675
                        time:   [63.498 µs 63.628 µs 63.772 µs]
                        thrpt:  [997.08 MiB/s 999.34 MiB/s 1001.4 MiB/s]
                 change:
                        time:   [+30.039% +32.273% +33.904%] (p = 0.00 < 0.05)
                        thrpt:  [-25.320% -24.399% -23.100%]
                        Performance has regressed.
FrameCompress/lz4_flex_rust_linked/66675
                        time:   [61.742 µs 61.866 µs 61.997 µs]
                        thrpt:  [1.0016 GiB/s 1.0037 GiB/s 1.0057 GiB/s]
                 change:
                        time:   [+32.540% +33.675% +34.914%] (p = 0.00 < 0.05)
                        thrpt:  [-25.878% -25.192% -24.551%]
                        Performance has regressed.
FrameCompress/lz4_flex_rust_indep/9991663
                        time:   [30.615 ms 30.660 ms 30.708 ms]
                        thrpt:  [310.31 MiB/s 310.79 MiB/s 311.25 MiB/s]
                 change:
                        time:   [-4.7967% -4.5869% -4.3892%] (p = 0.00 < 0.05)
                        thrpt:  [+4.5906% +4.8074% +5.0383%]
                        Performance has improved.
FrameCompress/lz4_flex_rust_linked/9991663
                        time:   [30.908 ms 30.953 ms 31.003 ms]
                        thrpt:  [307.35 MiB/s 307.85 MiB/s 308.30 MiB/s]
                 change:
                        time:   [-4.8133% -4.6088% -4.4012%] (p = 0.00 < 0.05)
                        thrpt:  [+4.6039% +4.8314% +5.0567%]
                        Performance has improved.
FrameCompress/lz4_flex_rust_indep/96274
                        time:   [12.696 µs 12.722 µs 12.750 µs]
                        thrpt:  [7.0324 GiB/s 7.0480 GiB/s 7.0621 GiB/s]
                 change:
                        time:   [+0.7671% +2.4321% +3.9652%] (p = 0.00 < 0.05)
                        thrpt:  [-3.8139% -2.3744% -0.7613%]
                        Change within noise threshold.
FrameCompress/lz4_flex_rust_linked/96274
                        time:   [13.905 µs 13.990 µs 14.103 µs]
                        thrpt:  [6.3577 GiB/s 6.4090 GiB/s 6.4481 GiB/s]
                 change:
                        time:   [-4.9845% -4.2791% -3.5052%] (p = 0.00 < 0.05)
                        thrpt:  [+3.6325% +4.4703% +5.2460%]
                        Performance has improved.

@PSeitz
Copy link
Owner

PSeitz commented Apr 6, 2023

Thanks! Shouldn't we at least print the error if something goes wrong?

I wouldn't have excepted such a large performance regression by wrapping the witer

@CosmicHorrorDev
Copy link
Contributor Author

I know the zip crate prints a message to stdout on error, but I think it's still pretty bad practice since it can mess with CLI programs. We could log a warning instead

The standard library typically just accepts that things will silently fail on drop and documents that you should call the finalization method instead (in our case .try_finish() could be used even for things like trait objects). Here's the snippet from std::io::BufWriter

It is critical to call flush before BufWriter<W> is dropped. Though dropping will attempt to flush the contents of the buffer, any errors that happen in the process of dropping will be ignored. Calling flush ensures that the buffer is empty and thus dropping will not even attempt file operations.

It's also easy enough for people to make their own wrapper that gives them better observability

struct LogOnDrop(FrameEncoder<File>);

impl Drop for LogOnDrop {
    fn drop(&mut self) {
        if let Err(e) = self.0.try_finish() {
            log::warn!("Failed finishing: {e}");
        }
    }
}

People can just use the finalization methods if they need to see that there is an error. Relying on drop for this kind of stuff always hinders how you can convey failures

@PSeitz
Copy link
Owner

PSeitz commented Apr 6, 2023

Okay, then let's document that, so users are aware they can't blindly use AutoFinishEncoder

@CosmicHorrorDev
Copy link
Contributor Author

Sounds good! I'll work on the documentation later today

@CosmicHorrorDev
Copy link
Contributor Author

I tweaked a lot of the documentation. Let me know if that's more to your liking

src/frame/compress.rs Outdated Show resolved Hide resolved
@PSeitz PSeitz merged commit 76a78ae into PSeitz:main Apr 7, 2023
2 checks passed
@PSeitz
Copy link
Owner

PSeitz commented Apr 7, 2023

Yes, thanks!

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.

None yet

2 participants