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

Implement bufwriter_raw_buffer feature #79921

Closed

Conversation

tgnottingham
Copy link
Contributor

@tgnottingham tgnottingham commented Dec 11, 2020

Tracking issue: #79916

@rust-highfive
Copy link
Collaborator

r? @shepmaster

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2020
@tgnottingham
Copy link
Contributor Author

I'll have a write-up for this in a few.

@tgnottingham
Copy link
Contributor Author

tgnottingham commented Dec 11, 2020

I've encountered some performance issues with BufWriter that made me think that it might benefit from having a lower-level API. Hoping to hear what others think about it.

The performance issue came up in a change I'm working on that uses BufWriter more heavily in rustc. The change is simple. It changes from writing to a Vec, then writing that Vec to a File, to writing directly to a BufWriter<File>.

The change makes a lot of small writes to BufWriter, which is exactly the use case for which it exists. So one would hope that the performance difference would be rather low. In fact, it's possible using BufWriter could be faster, as it doesn't have to deal with growing and reallocating its buffer, while Vec does. Let's try it out.

Result of switching from baseline to current BufWriter

bufwriter_unoptimized

Very definitely not faster, so I made some trivial changes to BufWriter to improve performance (#79930).

Result of switching from baseline to optimized BufWriter

bufwriter_optimized

Obviously, this is enormously better, but it still wasn't satisfying (well, it was very satisfying to improve BufWriter performance, but not satisfying that rustc perf was still bad). So I added the proposed low-level raw buffer API and used that.

Result of switching from baseline to raw buffer API BufWriter

bufwriter_raw_api

Better...closer...warmer. I can work with it anyway. Here is the raw buffer API vs the optimized BufWriter's existing API.

Result of switching from optimized BufWriter to raw BufWriter API

bufwriter_optimized_vs_raw_api

While the percentage differences seem small, remember that this is the performance difference for the entire compilation of these crates, not the BufWriter performance. An improvement of 4% likely represents many times the improvement in BufWriter performance.

So what is it about the existing API that accounts for the performance difference?

Well, one case in which a difference arises is when you have static knowledge about the input and BufWriter buffer sizes. With this knowledge, you can make repeated writes to the buffer in a way that eliminates bounds checks, errors checks, and all the incidental pessimizations that arise from having to include those checks.

BufWriter likely has no hope of making those optimizations, because it lacks this static knowledge. Even if it had this knowledge, we're generally at the mercy of the optimizer to see through the repeated calls to BufWriter and figure out that it can actually make the optimizations.

This was the use case I had for a raw buffer API, but there may be other cases where a client can do better with it. What do people think about adding this?

@rustbot label T-libs

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 11, 2020
@shepmaster
Copy link
Member

r? @sfackler

@rust-highfive rust-highfive assigned sfackler and unassigned shepmaster Jan 1, 2021
@sfackler
Copy link
Member

sfackler commented Jan 1, 2021

This seems like a strange API to expose from BufWriter, and whatever you're trying to do in rustc may be better served by a custom type.

@tgnottingham
Copy link
Contributor Author

tgnottingham commented Jan 1, 2021

@sfackler, that's the path I've taken in #80463, and I'm totally fine with it personally.

FWIW, the benefit of being able to use BufWriter instead of a custom type would be not having to reimplement flush and drop (in my case anyway). But that isn't enough for me to want to push for the proposed API.

@sfackler
Copy link
Member

sfackler commented Jan 1, 2021

Once the ReadBuf type is implemented, I think you could make an interface based off of that which would be a bit cleaner.

@tgnottingham
Copy link
Contributor Author

Thanks @sfackler. Closing this and the tracking issue.

@tgnottingham tgnottingham deleted the bufwriter_raw_buffer branch January 20, 2021 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants