Remove unnecessary Option from BufWriter

[apasel422]

Closes #36319

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[TimNN]

You are leaking the buffer here.

[apasel422]

Good catch!

[TimNN]

To be honest, I find using mem::replace here pretty confusing - in my opinion ptr::reading the buffer (maybe with a comment why) would convey the intent much better (dropping the buffer), especially since we rely on the fact that Vec::new() never allocates (which is document, but still).

[bluss]

Maybe std::ptr::drop_in_place(&mut self.buf) would be the clearest then (it does what it says), but I still want to favour safe code when it should be equivalent.

[TimNN]

Ah, now I understand why you prefer mem::replace. Both are fine with me I guess, although I think a short comment that buf is being dropped here probably wouldn't go amiss.

[durka]

drop_in_place won't work anymore with the removal of drop flags, right?

[bluss]

Sure it works

[durka]

Nevermind, I conflated it with read_and_drop.

On Thu, Sep 8, 2016 at 4:37 PM, bluss notifications@github.com wrote:

In src/libstd/io/buffered.rs
#36336 (comment):

@@ -451,7 +453,14 @@ impl<W: Write> BufWriter {
pub fn into_inner(mut self) -> Result<W, IntoInnerError<BufWriter>> {
match self.flush_buf() {
Err(e) => Err(IntoInnerError(self, e)),

•        Ok(()) => Ok(self.inner.take().unwrap())
  

•        Ok(()) => unsafe {
  

•            // use `ptr::read` to extract the inner writer and circumvent
  

•            // our `Drop` implementation
  

•            mem::replace(&mut self.buf, Vec::new());
  

Sure it works

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rust/pull/36336/files/ea72b7efc9f8a7fc2234264cbc6fc2241f66cd52#r78084035,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAC3nxeWhhJT2ZkrrBbzzvR9e2an8Wp3ks5qoHHzgaJpZM4J3ebB
.

[alexcrichton]

Thanks for the PR @apasel422! I'm curious though if we could get some perf numbers for this though? This is something that, while possible, is tricky to write, so it's best to have strong rationale for unsafe code here.

[bluss]

I was trying to look at perf only to discover that BufWriter is hit by extend_from_slice slowness too (#33518). (Let's fix Vec<u8>'s performance soon).

[bluss]

Ok that issue is resolved, great. I haven't looked at this in detail yet, but it's not in any inner loop, the option is only touched whenever the buffer is full. So it might be that it's not a great difference?

[alexcrichton]

My assumption is that this will be very difficult to get to show up in practice, and this is tricky enough that I'm not currently convinced the unsafety is worth it. If there's a strong perf case for doing it though, then I could be swayed!

[alexcrichton]

Discussed during libs triage our conclusion was to hold off on this until we can come up with some convincing benchmarks. @apasel422 you wouldn't happen to have any on hand would you?

[apasel422]

@alexcrichton I don't have any benchmarks right now. I'll reopen this once I do.

[czipperz]

I think I'm interested in implementing this, trying to do so without unsafe code if possible. @apasel422 ping from triage

[czipperz]

I'm getting these results for these benchmarks:

#[bench]
fn bench_buffered_writer(b: &mut test::Bencher) {
    b.iter(|| {
        BufWriter::new(io::sink())
    });
}

#[bench]
fn bench_buffered_writer_large_write_causing_flush(b: &mut test::Bencher) {
    b.iter(|| {
        let mut buf_writer = BufWriter::with_capacity(10, io::sink());
        buf_writer.write(b"abcdefghijklmnopqrstuvwxyz").unwrap();
    });
}

#[bench]
fn bench_buffered_writer_small_write(b: &mut test::Bencher) {
    b.iter(|| {
        let mut buf_writer = BufWriter::with_capacity(10, io::sink());
        buf_writer.write(b"abcdef").unwrap();
    });
}

#[bench]
fn bench_buffered_writer_multiple_small_writes(b: &mut test::Bencher) {
    b.iter(|| {
        let mut buf_writer = BufWriter::with_capacity(10, io::sink());
        buf_writer.write(b"abcdef").unwrap();
        buf_writer.write(b"abcdef").unwrap();
        buf_writer.write(b"abcdef").unwrap();
    });
}

* With option
|   Trial | construct | large write | multiple small | small |
|---------+-----------+-------------+----------------+-------|
|       1 |        81 |          66 |            134 |    83 |
|       2 |        81 |          67 |            134 |    83 |
|       3 |        82 |          67 |            134 |    82 |
|       4 |        82 |          68 |            134 |    82 |
|       5 |        82 |          68 |            135 |    86 |
| Average |      81.6 |        67.2 |          134.2 |  83.2 |

* Without option
|   Trial | construct | large write | multiple small | small |
|---------+-----------+-------------+----------------+-------|
|       1 |        80 |          65 |            131 |    80 |
|       2 |        79 |          65 |            132 |    79 |
|       3 |        80 |          66 |            133 |    80 |
|       4 |        80 |          66 |            133 |    80 |
|       5 |        81 |          65 |            134 |    81 |
| Average |      80.0 |        65.6 |          132.6 |  80.0 |

[czipperz]

My implementation is nearly the same, but into_inner is defined this way:

     pub fn into_inner(mut self) -> Result<W, IntoInnerError<BufWriter<W>>> {
         match self.flush_buf() {
             Err(e) => Err(IntoInnerError(self, e)),
-            Ok(()) => Ok(self.inner.take().unwrap())
+            Ok(()) => {
+                use std::mem::{replace, uninitialized, forget};
+                let inner = replace(&mut self.inner, unsafe { uninitialized() });
+                replace(&mut self.buf, unsafe { uninitialized() });
+                forget(self);
+                Ok(inner)
+            }
         }
     }