Optimize BufWriter

[tgnottingham]

No description provided.

[rust-highfive]

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

[tgnottingham]

This improves BufWriter performance by inlining the hot paths of write and write_all, not calling expensive Vec methods, and optimizing for the case where the input size is less than the buffer size.

Optimization was guided by rustc benchmarks. I have a local change that uses BufWriter more heavily in rustc. It changes code that wrote to a Vec, then wrote that Vec to a File, to code that writes directly to a BufWriter<File>.

Performance was initially completely unworkable.

Result of switching from baseline to current BufWriter

Here is the effect of adding each optimization to the current Bufwriter.

Optimization 1: inline hot paths
Optimization 2: avoid expensive Vec methods
Optimization 3: optimize for input size < buffer size

Result of Optimization 1 versus current BufWriter

Result of Optimization 1 + 2 versus Optimization 1

Result of Optimization 1 + 2 + 3 versus Optimization 1 + 2

Realize that these are benchmarks of the entirety of compilation, not just BufWriter code. So an increase in performance in the benchmarks represents a much larger improvement to BufWriter performance, at least according to rustc's use of it.

Obviously optimization 2 is the most impactful, optimization 1 is good, and optimization 3 would be good any other day of the week, but after the other two, it's like, whatever. :)

After all was said and done, the toll of using BufWriter heavily in rustc was far more tolerable:

Result of switching from baseline to optimized BufWriter

Still isn't quite where I want it to be though. See #79921 for details.

[tgnottingham]

I'm aware that #78551 is also making changes in BufWriter. I'm hoping that this PR and @Lucretiel's can be reconciled. I'm pretty certain the change to avoid using Vec methods can be, but I'm not as sure about the inlining and common case optimization parts.

[the8472]

Out of curiosity, have you experimented with a smaller hammer by using #[cold] or annotating the branches with likely/unlikely instead of #[inline(never)]?

[Kogia-sima]

I have a concern that this optimization is unsound when buf.len() >= isize::MAX. It may cause overflow, go into the fast path, and apply copy operation excessing the buffer capacity.

[tgnottingham]

Out of curiosity, have you experimented with a smaller hammer by using #[cold] or annotating the branches with likely/unlikely instead of #[inline(never)]?

I don't think I did--definitely not #[cold] anyway. I'll play with them.

Now that you mention it, I'm not quite sure what the right thing to do is here. If I don't separate the functions out, the rustc benchmarks take a pretty big hit. So I know that, for my modified rustc's use of BufWriter, it's better if they don't get inlined.

But maybe #[inline(never)] is too extreme? Maybe there are some situations where the optimizer can see that it would be better to inline them.

I could just remove the annotations, as currently, the "cold" functions don't get inlined in rustc, even without #[inline(never)]. But if they start getting inlined again as a result of some other change (and the lack of annotation), perf could take a hit for workloads where the inlining is harmful.

Anyway, I'm open to suggestions.

[tgnottingham]

I have a concern that this optimization is unsound when buf.len() >= isize::MAX. It may cause overflow, go into the fast path, and apply copy operation excessing the buffer capacity.

Ah, thank you. Looks like there was a correctness issue already, if I understand correctly, but it didn't lead to unsoundness until my change. I'll fix the problem.

Update: after looking into this, there's mostly no concern about overflow, as Vecs, slices, and objects in general are limited to isize::MAX bytes. Although there's no official spec for the language mandating this, I think it's de facto official, based on things like this:

The isize type is a signed integer type with the same number of bits as the platform's pointer type. The theoretical upper bound on object and array size is the maximum isize value. This ensures that isize can be used to calculate differences between pointers into an object or array and can address every byte within an object along with one byte past the end.

Even so, I tried using buf.len() < self.buf.capacity() - self.buf.len() to remove the question of addition overflow entirely. But the performance took a hit (this benchmark is super sensitive), and I'd like to avoid that.

There is potential for overflow in write_vectored, though, as it sums many slice lengths together. I'll try to address it.

Update again: disregard the strike-through text. I've learned that there's no guarantee yet that the maximum slice size won't increase beyond isize::MAX in the future. I've changed the code to safeguard against that possibility.

[tgnottingham]

I think I've addressed the possibility of overflow in write_vectored. It was a pre-existing issue, and so a bit outside the original scope of this PR. But I'm hoping that's alright, as fixing it is important for the soundness of the optimizations added by this PR.

[tgnottingham]

I've been told by @RalfJung that there's no guarantee yet that the maximum slice size won't increase beyond isize::MAX in the future. I'll update this PR to avoid the potential for overflow, even though it's not possible with the current language implementation.

[tgnottingham]

Out of curiosity, have you experimented with a smaller hammer by using #[cold] or annotating the branches with likely/unlikely instead of #[inline(never)]?

I don't think I did--definitely not #[cold] anyway. I'll play with them.

Btw, recombining the hot/cold code and annotating the branches wasn't helpful. And using #[inline(never)] versus #[cold] versus no annotation at all didn't change the inlining decisions, at least for the case I'm testing. Still not sure what the best course of action is there.

[the8472]

#[cold] feels more appropriate semantically, but other places in the standard library also use #[inline(never)] as an optimization against code bloat so I guess it doesn't really matter.

[mzabaluev]

#[cold] and #[inline] serve different purposes. #[cold] hints the code generator to set branch prediction bias, if supported by the target architecture. I think the _cold functions should have both #[cold] and #[inline(never)].

[mzabaluev]

I think the _cold functions should have both #[cold] and #[inline(never)].

Or perhaps, just #[cold] and let the optimizer decide about inlining. Same applies to #[inline(always)]; this attribute is prone to be misused.

[tgnottingham]

Okay, I've rebased, added a comment about the overflow edge case discussed above, and changed #[inline(never)] and #[inline(always)] to #[cold] and #[inline] respectively.

[tgnottingham]

Rebased on latest master. Will address comments soon.

[JohnCSimon]

@rustbot label: +S-waiting-on-review -S-waiting-on-author

[tgnottingham]

This change makes me wonder whether we should try to extend the interface of Vec to support use cases like this where you don't want it to grow (e.g. Vec::try_push or Vec::extend_from_slice_unchecked, etc.). Or if we should change Vec::spare_capacity_mut (which is still unstable) to return some wrapper type that doesn't only give you a &mut [MaybeUninit], but also allows updating len somehow. Or maybe a separate BoundedVec type would make more sense.

What do you think?

In this case, I think it's easy enough to get by with a Box<[MaybeUninit<u8>]. In general, I think a non-growing Vector might be nice to have, but it would have to be judged against whether or not it's worth adding to the Vec API (which is not something I think I'm qualified to evaluate! :)). Seems a bit fringe, but it does come up from time to time.

[m-ou-se]

(Note that you could also implement this with self.buf.spare_capacity_mut() and MaybeUninit::write_slice, which are both still unstable.)

[m-ou-se]

Thanks a lot for doing this!

@bors r+

[bors]

📌 Commit 01e7018 has been approved by m-ou-se

[Mark-Simulacrum]

@bors rollup=never - performance PR

[bors]

⌛ Testing commit 01e7018 with merge 676ee14...

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 676ee14 to master...