From 1f32d40ac3c51d6867fc6969e52e960452a40652 Mon Sep 17 00:00:00 2001 From: Tyson Nottingham Date: Thu, 10 Dec 2020 19:46:54 -0800 Subject: [PATCH 1/7] BufWriter: apply #[inline] / #[inline(never)] optimizations Ensure that `write` and `write_all` can be inlined and that their commonly executed fast paths can be as short as possible. `write_vectored` would likely benefit from the same optimization, but I omitted it because its implementation is more complex, and I don't have a benchmark on hand to guide its optimization. --- library/std/src/io/buffered/bufwriter.rs | 90 +++++++++++++++++------- 1 file changed, 66 insertions(+), 24 deletions(-) diff --git a/library/std/src/io/buffered/bufwriter.rs b/library/std/src/io/buffered/bufwriter.rs index 80f98bbbad366..ae72ddabfb9e4 100644 --- a/library/std/src/io/buffered/bufwriter.rs +++ b/library/std/src/io/buffered/bufwriter.rs @@ -331,6 +331,52 @@ impl BufWriter { let buf = if !self.panicked { Ok(buf) } else { Err(WriterPanicked { buf }) }; (self.inner.take().unwrap(), buf) } + + // Ensure this function does not get inlined into `write`, so that it + // remains inlineable and its common path remains as short as possible. + // If this function ends up being called frequently relative to `write`, + // it's likely a sign that the client is using an improperly sized buffer. + #[inline(never)] + fn flush_and_write(&mut self, buf: &[u8]) -> io::Result { + self.flush_buf()?; + + // Why not len > capacity? To avoid a needless trip through the buffer when the + // input exactly fills. We'd just need to flush it to the underlying writer anyway. + if buf.len() >= self.buf.capacity() { + self.panicked = true; + let r = self.get_mut().write(buf); + self.panicked = false; + r + } else { + self.buf.extend_from_slice(buf); + Ok(buf.len()) + } + } + + // Ensure this function does not get inlined into `write_all`, so that it + // remains inlineable and its common path remains as short as possible. + // If this function ends up being called frequently relative to `write_all`, + // it's likely a sign that the client is using an improperly sized buffer. + #[inline(never)] + fn flush_and_write_all(&mut self, buf: &[u8]) -> io::Result<()> { + // Normally, `write_all` just calls `write` in a loop. We can do better + // by calling `self.get_mut().write_all()` directly, which avoids + // round trips through the buffer in the event of a series of partial + // writes in some circumstances. + self.flush_buf()?; + + // Why not len > capacity? To avoid a needless trip through the buffer when the + // input exactly fills. We'd just need to flush it to the underlying writer anyway. + if buf.len() >= self.buf.capacity() { + self.panicked = true; + let r = self.get_mut().write_all(buf); + self.panicked = false; + r + } else { + self.buf.extend_from_slice(buf); + Ok(()) + } + } } #[unstable(feature = "bufwriter_into_raw_parts", issue = "80690")] @@ -402,43 +448,39 @@ impl fmt::Debug for WriterPanicked { #[stable(feature = "rust1", since = "1.0.0")] impl Write for BufWriter { + #[inline] fn write(&mut self, buf: &[u8]) -> io::Result { - if self.buf.len() + buf.len() > self.buf.capacity() { - self.flush_buf()?; - } - // FIXME: Why no len > capacity? Why not buffer len == capacity? #72919 - if buf.len() >= self.buf.capacity() { - self.panicked = true; - let r = self.get_mut().write(buf); - self.panicked = false; - r - } else { + // The `buf.len() != self.buf.capacity()` check is done to avoid a needless trip through + // the buffer when the input is exactly the same size as it. For many clients, that is a + // rare event, so it's unfortunate that the check is in the common code path. But it + // prevents pathological cases for other clients which *always* make writes of this size. + // See #72919 and #79930 for more info and a breadcrumb trail. + if self.buf.len() + buf.len() <= self.buf.capacity() && buf.len() != self.buf.capacity() { self.buf.extend_from_slice(buf); Ok(buf.len()) + } else { + self.flush_and_write(buf) } } + #[inline] fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { - // Normally, `write_all` just calls `write` in a loop. We can do better - // by calling `self.get_mut().write_all()` directly, which avoids - // round trips through the buffer in the event of a series of partial - // writes in some circumstances. - if self.buf.len() + buf.len() > self.buf.capacity() { - self.flush_buf()?; - } - // FIXME: Why no len > capacity? Why not buffer len == capacity? #72919 - if buf.len() >= self.buf.capacity() { - self.panicked = true; - let r = self.get_mut().write_all(buf); - self.panicked = false; - r - } else { + // The `buf.len() != self.buf.capacity()` check is done to avoid a needless trip through + // the buffer when the input is exactly the same size as it. For many clients, that is a + // rare event, so it's unfortunate that the check is in the common code path. But it + // prevents pathological cases for other clients which *always* make writes of this size. + // See #72919 and #79930 for more info and a breadcrumb trail. + if self.buf.len() + buf.len() <= self.buf.capacity() && buf.len() != self.buf.capacity() { self.buf.extend_from_slice(buf); Ok(()) + } else { + self.flush_and_write_all(buf) } } fn write_vectored(&mut self, bufs: &[IoSlice<'_>]) -> io::Result { + // FIXME: Consider applying `#[inline]` / `#[inline(never)]` optimizations already applied + // to `write` and `write_all`. The performance benefits can be significant. See #79930. if self.get_ref().is_write_vectored() { let total_len = bufs.iter().map(|b| b.len()).sum::(); if self.buf.len() + total_len > self.buf.capacity() { From b43e8e248bfba9e8d26821c6d98deba94d024abf Mon Sep 17 00:00:00 2001 From: Tyson Nottingham Date: Thu, 10 Dec 2020 21:34:28 -0800 Subject: [PATCH 2/7] BufWriter: avoid using expensive Vec methods We use a Vec as our internal, constant-sized buffer, but the overhead of using methods like `extend_from_slice` can be enormous, likely because they don't get inlined, because `Vec` has to repeat bounds checks that we've already done, and because it makes considerations for things like reallocating, even though they should never happen. --- library/std/src/io/buffered/bufwriter.rs | 87 ++++++++++++++++++++---- 1 file changed, 75 insertions(+), 12 deletions(-) diff --git a/library/std/src/io/buffered/bufwriter.rs b/library/std/src/io/buffered/bufwriter.rs index ae72ddabfb9e4..3272826d77c25 100644 --- a/library/std/src/io/buffered/bufwriter.rs +++ b/library/std/src/io/buffered/bufwriter.rs @@ -4,6 +4,7 @@ use crate::io::{ self, Error, ErrorKind, IntoInnerError, IoSlice, Seek, SeekFrom, Write, DEFAULT_BUF_SIZE, }; use crate::mem; +use crate::ptr; /// Wraps a writer and buffers its output. /// @@ -68,6 +69,10 @@ use crate::mem; #[stable(feature = "rust1", since = "1.0.0")] pub struct BufWriter { inner: Option, + // The buffer. Avoid using this like a normal `Vec` in common code paths. + // That is, don't use `buf.push`, `buf.extend_from_slice`, or any other + // methods that require bounds checking or the like. This makes an enormous + // difference to performance (we may want to stop using a `Vec` entirely). buf: Vec, // #30888: If the inner writer panics in a call to write, we don't want to // write the buffered data a second time in BufWriter's destructor. This @@ -150,7 +155,11 @@ impl BufWriter { impl Drop for BufGuard<'_> { fn drop(&mut self) { if self.written > 0 { - self.buffer.drain(..self.written); + if self.done() { + self.buffer.clear(); + } else { + self.buffer.drain(..self.written); + } } } } @@ -183,7 +192,12 @@ impl BufWriter { pub(super) fn write_to_buf(&mut self, buf: &[u8]) -> usize { let available = self.buf.capacity() - self.buf.len(); let amt_to_buffer = available.min(buf.len()); - self.buf.extend_from_slice(&buf[..amt_to_buffer]); + + // SAFETY: `amt_to_buffer` is <= buffer's spare capacity by construction. + unsafe { + self.write_to_buffer_unchecked(&buf[..amt_to_buffer]); + } + amt_to_buffer } @@ -348,7 +362,13 @@ impl BufWriter { self.panicked = false; r } else { - self.buf.extend_from_slice(buf); + // SAFETY: We just called `self.flush_buf()`, so `self.buf.len()` is 0, and + // we entered this else block because `buf.len() < self.buf.capacity()`. + // Therefore, `self.buf.len() + buf.len() <= self.buf.capacity()`. + unsafe { + self.write_to_buffer_unchecked(buf); + } + Ok(buf.len()) } } @@ -373,10 +393,29 @@ impl BufWriter { self.panicked = false; r } else { - self.buf.extend_from_slice(buf); + // SAFETY: We just called `self.flush_buf()`, so `self.buf.len()` is 0, and + // we entered this else block because `buf.len() < self.buf.capacity()`. + // Therefore, `self.buf.len() + buf.len() <= self.buf.capacity()`. + unsafe { + self.write_to_buffer_unchecked(buf); + } + Ok(()) } } + + // SAFETY: Requires `self.buf.len() + buf.len() <= self.buf.capacity()`, + // i.e., that input buffer length is less than or equal to spare capacity. + #[inline(always)] + unsafe fn write_to_buffer_unchecked(&mut self, buf: &[u8]) { + debug_assert!(self.buf.len() + buf.len() <= self.buf.capacity()); + let old_len = self.buf.len(); + let buf_len = buf.len(); + let src = buf.as_ptr(); + let dst = self.buf.as_mut_ptr().add(old_len); + ptr::copy_nonoverlapping(src, dst, buf_len); + self.buf.set_len(old_len + buf_len); + } } #[unstable(feature = "bufwriter_into_raw_parts", issue = "80690")] @@ -456,7 +495,11 @@ impl Write for BufWriter { // prevents pathological cases for other clients which *always* make writes of this size. // See #72919 and #79930 for more info and a breadcrumb trail. if self.buf.len() + buf.len() <= self.buf.capacity() && buf.len() != self.buf.capacity() { - self.buf.extend_from_slice(buf); + // SAFETY: safe by above conditional. + unsafe { + self.write_to_buffer_unchecked(buf); + } + Ok(buf.len()) } else { self.flush_and_write(buf) @@ -471,7 +514,11 @@ impl Write for BufWriter { // prevents pathological cases for other clients which *always* make writes of this size. // See #72919 and #79930 for more info and a breadcrumb trail. if self.buf.len() + buf.len() <= self.buf.capacity() && buf.len() != self.buf.capacity() { - self.buf.extend_from_slice(buf); + // SAFETY: safe by above conditional. + unsafe { + self.write_to_buffer_unchecked(buf); + } + Ok(()) } else { self.flush_and_write_all(buf) @@ -492,7 +539,13 @@ impl Write for BufWriter { self.panicked = false; r } else { - bufs.iter().for_each(|b| self.buf.extend_from_slice(b)); + // SAFETY: We checked whether or not the spare capacity was large enough above. If + // it was, then we're safe already. If it wasn't, we flushed, making sufficient + // room for any input <= the buffer size, which includes this input. + unsafe { + bufs.iter().for_each(|b| self.write_to_buffer_unchecked(b)); + }; + Ok(total_len) } } else { @@ -511,7 +564,13 @@ impl Write for BufWriter { self.panicked = false; return r; } else { - self.buf.extend_from_slice(buf); + // SAFETY: We checked whether or not the spare capacity was large enough above. + // If it was, then we're safe already. If it wasn't, we flushed, making + // sufficient room for any input <= the buffer size, which includes this input. + unsafe { + self.write_to_buffer_unchecked(buf); + } + buf.len() } } else { @@ -519,11 +578,15 @@ impl Write for BufWriter { }; debug_assert!(total_written != 0); for buf in iter { - if self.buf.len() + buf.len() > self.buf.capacity() { - break; - } else { - self.buf.extend_from_slice(buf); + if self.buf.len() + buf.len() <= self.buf.capacity() { + // SAFETY: safe by above conditional. + unsafe { + self.write_to_buffer_unchecked(buf); + } + total_written += buf.len(); + } else { + break; } } Ok(total_written) From 5fd9372c1118b213b15a0e75bec7fab3b2e00260 Mon Sep 17 00:00:00 2001 From: Tyson Nottingham Date: Sat, 12 Dec 2020 23:17:05 -0800 Subject: [PATCH 3/7] BufWriter: optimize for write sizes less than buffer size Optimize for the common case where the input write size is less than the buffer size. This slightly increases the cost for pathological write patterns that commonly fill the buffer exactly, but if a client is doing that frequently, they're already paying the cost of frequent flushing, etc., so the cost is of this optimization to them is relatively small. --- library/std/src/io/buffered/bufwriter.rs | 56 ++++++++++++++---------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/library/std/src/io/buffered/bufwriter.rs b/library/std/src/io/buffered/bufwriter.rs index 3272826d77c25..f0ff186d99b11 100644 --- a/library/std/src/io/buffered/bufwriter.rs +++ b/library/std/src/io/buffered/bufwriter.rs @@ -349,19 +349,26 @@ impl BufWriter { // Ensure this function does not get inlined into `write`, so that it // remains inlineable and its common path remains as short as possible. // If this function ends up being called frequently relative to `write`, - // it's likely a sign that the client is using an improperly sized buffer. + // it's likely a sign that the client is using an improperly sized buffer + // or their write patterns are somewhat pathological. #[inline(never)] - fn flush_and_write(&mut self, buf: &[u8]) -> io::Result { - self.flush_buf()?; + fn write_cold(&mut self, buf: &[u8]) -> io::Result { + if self.buf.len() + buf.len() > self.buf.capacity() { + self.flush_buf()?; + } - // Why not len > capacity? To avoid a needless trip through the buffer when the - // input exactly fills. We'd just need to flush it to the underlying writer anyway. + // Why not len > capacity? To avoid a needless trip through the buffer when the input + // exactly fills it. We'd just need to flush it to the underlying writer anyway. if buf.len() >= self.buf.capacity() { self.panicked = true; let r = self.get_mut().write(buf); self.panicked = false; r } else { + // Write to the buffer. In this case, we write to the buffer even if it fills it + // exactly. Doing otherwise would mean flushing the buffer, then writing this + // input to the inner writer, which in many cases would be a worse strategy. + // SAFETY: We just called `self.flush_buf()`, so `self.buf.len()` is 0, and // we entered this else block because `buf.len() < self.buf.capacity()`. // Therefore, `self.buf.len() + buf.len() <= self.buf.capacity()`. @@ -376,23 +383,30 @@ impl BufWriter { // Ensure this function does not get inlined into `write_all`, so that it // remains inlineable and its common path remains as short as possible. // If this function ends up being called frequently relative to `write_all`, - // it's likely a sign that the client is using an improperly sized buffer. + // it's likely a sign that the client is using an improperly sized buffer + // or their write patterns are somewhat pathological. #[inline(never)] - fn flush_and_write_all(&mut self, buf: &[u8]) -> io::Result<()> { + fn write_all_cold(&mut self, buf: &[u8]) -> io::Result<()> { // Normally, `write_all` just calls `write` in a loop. We can do better // by calling `self.get_mut().write_all()` directly, which avoids // round trips through the buffer in the event of a series of partial // writes in some circumstances. - self.flush_buf()?; + if self.buf.len() + buf.len() > self.buf.capacity() { + self.flush_buf()?; + } - // Why not len > capacity? To avoid a needless trip through the buffer when the - // input exactly fills. We'd just need to flush it to the underlying writer anyway. + // Why not len > capacity? To avoid a needless trip through the buffer when the input + // exactly fills it. We'd just need to flush it to the underlying writer anyway. if buf.len() >= self.buf.capacity() { self.panicked = true; let r = self.get_mut().write_all(buf); self.panicked = false; r } else { + // Write to the buffer. In this case, we write to the buffer even if it fills it + // exactly. Doing otherwise would mean flushing the buffer, then writing this + // input to the inner writer, which in many cases would be a worse strategy. + // SAFETY: We just called `self.flush_buf()`, so `self.buf.len()` is 0, and // we entered this else block because `buf.len() < self.buf.capacity()`. // Therefore, `self.buf.len() + buf.len() <= self.buf.capacity()`. @@ -489,12 +503,9 @@ impl fmt::Debug for WriterPanicked { impl Write for BufWriter { #[inline] fn write(&mut self, buf: &[u8]) -> io::Result { - // The `buf.len() != self.buf.capacity()` check is done to avoid a needless trip through - // the buffer when the input is exactly the same size as it. For many clients, that is a - // rare event, so it's unfortunate that the check is in the common code path. But it - // prevents pathological cases for other clients which *always* make writes of this size. - // See #72919 and #79930 for more info and a breadcrumb trail. - if self.buf.len() + buf.len() <= self.buf.capacity() && buf.len() != self.buf.capacity() { + // Use < instead of <= to avoid a needless trip through the buffer in some cases. + // See `write_cold` for details. + if self.buf.len() + buf.len() < self.buf.capacity() { // SAFETY: safe by above conditional. unsafe { self.write_to_buffer_unchecked(buf); @@ -502,18 +513,15 @@ impl Write for BufWriter { Ok(buf.len()) } else { - self.flush_and_write(buf) + self.write_cold(buf) } } #[inline] fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { - // The `buf.len() != self.buf.capacity()` check is done to avoid a needless trip through - // the buffer when the input is exactly the same size as it. For many clients, that is a - // rare event, so it's unfortunate that the check is in the common code path. But it - // prevents pathological cases for other clients which *always* make writes of this size. - // See #72919 and #79930 for more info and a breadcrumb trail. - if self.buf.len() + buf.len() <= self.buf.capacity() && buf.len() != self.buf.capacity() { + // Use < instead of <= to avoid a needless trip through the buffer in some cases. + // See `write_all_cold` for details. + if self.buf.len() + buf.len() < self.buf.capacity() { // SAFETY: safe by above conditional. unsafe { self.write_to_buffer_unchecked(buf); @@ -521,7 +529,7 @@ impl Write for BufWriter { Ok(()) } else { - self.flush_and_write_all(buf) + self.write_all_cold(buf) } } From 72aecbfd01e73da710ee35826f28f41d5d0cfebe Mon Sep 17 00:00:00 2001 From: Tyson Nottingham Date: Tue, 15 Dec 2020 14:18:02 -0800 Subject: [PATCH 4/7] BufWriter: handle possibility of overflow --- library/std/src/io/buffered/bufwriter.rs | 54 +++++++++++++++++------- 1 file changed, 39 insertions(+), 15 deletions(-) diff --git a/library/std/src/io/buffered/bufwriter.rs b/library/std/src/io/buffered/bufwriter.rs index f0ff186d99b11..a9fc450de3182 100644 --- a/library/std/src/io/buffered/bufwriter.rs +++ b/library/std/src/io/buffered/bufwriter.rs @@ -190,7 +190,7 @@ impl BufWriter { /// data. Writes as much as possible without exceeding capacity. Returns /// the number of bytes written. pub(super) fn write_to_buf(&mut self, buf: &[u8]) -> usize { - let available = self.buf.capacity() - self.buf.len(); + let available = self.spare_capacity(); let amt_to_buffer = available.min(buf.len()); // SAFETY: `amt_to_buffer` is <= buffer's spare capacity by construction. @@ -353,7 +353,7 @@ impl BufWriter { // or their write patterns are somewhat pathological. #[inline(never)] fn write_cold(&mut self, buf: &[u8]) -> io::Result { - if self.buf.len() + buf.len() > self.buf.capacity() { + if buf.len() > self.spare_capacity() { self.flush_buf()?; } @@ -371,7 +371,7 @@ impl BufWriter { // SAFETY: We just called `self.flush_buf()`, so `self.buf.len()` is 0, and // we entered this else block because `buf.len() < self.buf.capacity()`. - // Therefore, `self.buf.len() + buf.len() <= self.buf.capacity()`. + // Therefore, `buf.len() <= self.buf.capacity() - self.buf.len()`. unsafe { self.write_to_buffer_unchecked(buf); } @@ -391,7 +391,8 @@ impl BufWriter { // by calling `self.get_mut().write_all()` directly, which avoids // round trips through the buffer in the event of a series of partial // writes in some circumstances. - if self.buf.len() + buf.len() > self.buf.capacity() { + + if buf.len() > self.spare_capacity() { self.flush_buf()?; } @@ -409,7 +410,7 @@ impl BufWriter { // SAFETY: We just called `self.flush_buf()`, so `self.buf.len()` is 0, and // we entered this else block because `buf.len() < self.buf.capacity()`. - // Therefore, `self.buf.len() + buf.len() <= self.buf.capacity()`. + // Therefore, `buf.len() <= self.buf.capacity() - self.buf.len()`. unsafe { self.write_to_buffer_unchecked(buf); } @@ -418,11 +419,11 @@ impl BufWriter { } } - // SAFETY: Requires `self.buf.len() + buf.len() <= self.buf.capacity()`, + // SAFETY: Requires `buf.len() <= self.buf.capacity() - self.buf.len()`, // i.e., that input buffer length is less than or equal to spare capacity. #[inline(always)] unsafe fn write_to_buffer_unchecked(&mut self, buf: &[u8]) { - debug_assert!(self.buf.len() + buf.len() <= self.buf.capacity()); + debug_assert!(buf.len() <= self.spare_capacity()); let old_len = self.buf.len(); let buf_len = buf.len(); let src = buf.as_ptr(); @@ -430,6 +431,11 @@ impl BufWriter { ptr::copy_nonoverlapping(src, dst, buf_len); self.buf.set_len(old_len + buf_len); } + + #[inline] + fn spare_capacity(&self) -> usize { + self.buf.capacity() - self.buf.len() + } } #[unstable(feature = "bufwriter_into_raw_parts", issue = "80690")] @@ -505,7 +511,7 @@ impl Write for BufWriter { fn write(&mut self, buf: &[u8]) -> io::Result { // Use < instead of <= to avoid a needless trip through the buffer in some cases. // See `write_cold` for details. - if self.buf.len() + buf.len() < self.buf.capacity() { + if buf.len() < self.spare_capacity() { // SAFETY: safe by above conditional. unsafe { self.write_to_buffer_unchecked(buf); @@ -521,7 +527,7 @@ impl Write for BufWriter { fn write_all(&mut self, buf: &[u8]) -> io::Result<()> { // Use < instead of <= to avoid a needless trip through the buffer in some cases. // See `write_all_cold` for details. - if self.buf.len() + buf.len() < self.buf.capacity() { + if buf.len() < self.spare_capacity() { // SAFETY: safe by above conditional. unsafe { self.write_to_buffer_unchecked(buf); @@ -537,16 +543,31 @@ impl Write for BufWriter { // FIXME: Consider applying `#[inline]` / `#[inline(never)]` optimizations already applied // to `write` and `write_all`. The performance benefits can be significant. See #79930. if self.get_ref().is_write_vectored() { - let total_len = bufs.iter().map(|b| b.len()).sum::(); - if self.buf.len() + total_len > self.buf.capacity() { + // We have to handle the possibility that the total length of the buffers overflows + // `usize` (even though this can only happen if multiple `IoSlice`s reference the + // same underlying buffer, as otherwise the buffers wouldn't fit in memory). If the + // computation overflows, then surely the input cannot fit in our buffer, so we forward + // to the inner writer's `write_vectored` method to let it handle it appropriately. + let saturated_total_len = + bufs.iter().fold(0usize, |acc, b| acc.saturating_add(b.len())); + + if saturated_total_len > self.spare_capacity() { + // Flush if the total length of the input exceeds our buffer's spare capacity. + // If we would have overflowed, this condition also holds, and we need to flush. self.flush_buf()?; } - if total_len >= self.buf.capacity() { + + if saturated_total_len >= self.buf.capacity() { + // Forward to our inner writer if the total length of the input is greater than or + // equal to our buffer capacity. If we would have overflowed, this condition also + // holds, and we punt to the inner writer. self.panicked = true; let r = self.get_mut().write_vectored(bufs); self.panicked = false; r } else { + // `saturated_total_len < self.buf.capacity()` implies that we did not saturate. + // SAFETY: We checked whether or not the spare capacity was large enough above. If // it was, then we're safe already. If it wasn't, we flushed, making sufficient // room for any input <= the buffer size, which includes this input. @@ -554,14 +575,14 @@ impl Write for BufWriter { bufs.iter().for_each(|b| self.write_to_buffer_unchecked(b)); }; - Ok(total_len) + Ok(saturated_total_len) } } else { let mut iter = bufs.iter(); let mut total_written = if let Some(buf) = iter.by_ref().find(|&buf| !buf.is_empty()) { // This is the first non-empty slice to write, so if it does // not fit in the buffer, we still get to flush and proceed. - if self.buf.len() + buf.len() > self.buf.capacity() { + if buf.len() > self.spare_capacity() { self.flush_buf()?; } if buf.len() >= self.buf.capacity() { @@ -586,12 +607,15 @@ impl Write for BufWriter { }; debug_assert!(total_written != 0); for buf in iter { - if self.buf.len() + buf.len() <= self.buf.capacity() { + if buf.len() <= self.spare_capacity() { // SAFETY: safe by above conditional. unsafe { self.write_to_buffer_unchecked(buf); } + // This cannot overflow `usize`. If we are here, we've written all of the bytes + // so far to our buffer, and we've ensured that we never exceed the buffer's + // capacity. Therefore, `total_written` <= `self.buf.capacity()` <= `usize::MAX`. total_written += buf.len(); } else { break; From 85bc88df5f97d31cf6297865dcd6092ba12b740b Mon Sep 17 00:00:00 2001 From: Tyson Nottingham Date: Mon, 4 Jan 2021 17:53:23 -0800 Subject: [PATCH 5/7] BufWriter: use #[cold] and less aggressive #[inline] hints --- library/std/src/io/buffered/bufwriter.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/std/src/io/buffered/bufwriter.rs b/library/std/src/io/buffered/bufwriter.rs index a9fc450de3182..9dbf0e7454d7f 100644 --- a/library/std/src/io/buffered/bufwriter.rs +++ b/library/std/src/io/buffered/bufwriter.rs @@ -351,6 +351,7 @@ impl BufWriter { // If this function ends up being called frequently relative to `write`, // it's likely a sign that the client is using an improperly sized buffer // or their write patterns are somewhat pathological. + #[cold] #[inline(never)] fn write_cold(&mut self, buf: &[u8]) -> io::Result { if buf.len() > self.spare_capacity() { @@ -385,6 +386,7 @@ impl BufWriter { // If this function ends up being called frequently relative to `write_all`, // it's likely a sign that the client is using an improperly sized buffer // or their write patterns are somewhat pathological. + #[cold] #[inline(never)] fn write_all_cold(&mut self, buf: &[u8]) -> io::Result<()> { // Normally, `write_all` just calls `write` in a loop. We can do better @@ -421,7 +423,7 @@ impl BufWriter { // SAFETY: Requires `buf.len() <= self.buf.capacity() - self.buf.len()`, // i.e., that input buffer length is less than or equal to spare capacity. - #[inline(always)] + #[inline] unsafe fn write_to_buffer_unchecked(&mut self, buf: &[u8]) { debug_assert!(buf.len() <= self.spare_capacity()); let old_len = self.buf.len(); From 0f29dc40f837b0a491d183ef05dd00eb78b6be1d Mon Sep 17 00:00:00 2001 From: Tyson Nottingham Date: Tue, 13 Apr 2021 10:09:37 -0700 Subject: [PATCH 6/7] BufWriter: simplify buffer draining --- library/std/src/io/buffered/bufwriter.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/library/std/src/io/buffered/bufwriter.rs b/library/std/src/io/buffered/bufwriter.rs index 9dbf0e7454d7f..5c5f4467ef950 100644 --- a/library/std/src/io/buffered/bufwriter.rs +++ b/library/std/src/io/buffered/bufwriter.rs @@ -155,11 +155,7 @@ impl BufWriter { impl Drop for BufGuard<'_> { fn drop(&mut self) { if self.written > 0 { - if self.done() { - self.buffer.clear(); - } else { - self.buffer.drain(..self.written); - } + self.buffer.drain(..self.written); } } } From 01e701828c6010873def5d79191319a6b37a6b32 Mon Sep 17 00:00:00 2001 From: Tyson Nottingham Date: Tue, 13 Apr 2021 21:09:41 -0700 Subject: [PATCH 7/7] BufWriter: improve safety comment --- library/std/src/io/buffered/bufwriter.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/library/std/src/io/buffered/bufwriter.rs b/library/std/src/io/buffered/bufwriter.rs index 5c5f4467ef950..ef2769d431fbb 100644 --- a/library/std/src/io/buffered/bufwriter.rs +++ b/library/std/src/io/buffered/bufwriter.rs @@ -366,9 +366,11 @@ impl BufWriter { // exactly. Doing otherwise would mean flushing the buffer, then writing this // input to the inner writer, which in many cases would be a worse strategy. - // SAFETY: We just called `self.flush_buf()`, so `self.buf.len()` is 0, and - // we entered this else block because `buf.len() < self.buf.capacity()`. - // Therefore, `buf.len() <= self.buf.capacity() - self.buf.len()`. + // SAFETY: There was either enough spare capacity already, or there wasn't and we + // flushed the buffer to ensure that there is. In the latter case, we know that there + // is because flushing ensured that our entire buffer is spare capacity, and we entered + // this block because the input buffer length is less than that capacity. In either + // case, it's safe to write the input buffer to our buffer. unsafe { self.write_to_buffer_unchecked(buf); } @@ -406,9 +408,11 @@ impl BufWriter { // exactly. Doing otherwise would mean flushing the buffer, then writing this // input to the inner writer, which in many cases would be a worse strategy. - // SAFETY: We just called `self.flush_buf()`, so `self.buf.len()` is 0, and - // we entered this else block because `buf.len() < self.buf.capacity()`. - // Therefore, `buf.len() <= self.buf.capacity() - self.buf.len()`. + // SAFETY: There was either enough spare capacity already, or there wasn't and we + // flushed the buffer to ensure that there is. In the latter case, we know that there + // is because flushing ensured that our entire buffer is spare capacity, and we entered + // this block because the input buffer length is less than that capacity. In either + // case, it's safe to write the input buffer to our buffer. unsafe { self.write_to_buffer_unchecked(buf); }