Closed CosmicHorrorDev closed 1 year ago
Merging #100 (10ecc5d) into main (be2ea76) will decrease coverage by
0.87%
. The diff coverage is0.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%) |
:arrow_up: |
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.
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.
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
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
beforeBufWriter<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. Callingflush
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
Okay, then let's document that, so users are aware they can't blindly use AutoFinishEncoder
Sounds good! I'll work on the documentation later today
I tweaked a lot of the documentation. Let me know if that's more to your liking
Yes, thanks!
This makes the
Drop
impl forAutoFinishEncoder<_>
infallible. Panicking ondrop
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 panickingI tried making
Encoder
automatically call.try_finish()
on drop, so thatAutoFinishEncoder<_>
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 (seezip
,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 anOption<_>
, so that it can be.take
n by methods like.into_inner()
while still providing a drop impl