Fix perf regression in Read::read_to_end on short reads due to not checking if the cursor has initialized bytes

[asder8215]

View all comments

This PR fixes #158008.

In particular, in #150129, it refactored some code within library/std/io/mod.rs to utilize BorrowedBuf::is_init instead of manually checking read_buf.init_len() == buf_len to see if the read buffer had initialized bytes. However, the BorrowedBuf is never marked or set as init within this function, and I think this portion of the code:

 // SAFETY: These bytes were initialized but not filled in the previous loop
unsafe {
     read_buf.set_init(initialized);
}

was removed by mistake. This PR reverts the changes made by #150129, so that we can mark the BorrowedBuf/read_buf as initialized using BorrowedBuf::set_init if in a previous iteration the cursor has initialized bytes. This would allow max_read_size to not be marked as usize::max if the read buffer contains initialized bytes.

[rustbot]

r? @Darksonn

rustbot has assigned @Darksonn.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 12 candidates
• Random selection from Darksonn, Mark-Simulacrum, clarfonthey, jhpratt

[Darksonn]

This comment was added in #150129. Should it be removed?

View changes since the review

[asder8215]

Unsure. Are these comments still relevant @a1phyr?

[a1phyr]

Well, probably not if you start tracking initialized bytes :)

[Darksonn]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[a1phyr]

I didn't track uninitialized bytes in my previous MR because I thought it would be to complicated to do properly.

For example, if this MR improve some existing cases, it will not really solve the pathological case you sent in your issue for larger sizes (eg around a million): when you initialized N bytes, on the next round you will have N-1 spare initialized bytes left, so you won't be able to use set_init() (or you could initialize the rest manually).

All in all, it was a trade-off between code complexity, properly handling common cases but having suboptimal (but still acceptable) behavior in weird cases.

View changes since this review

[a1phyr]

This is true only if read_buf.is_init() (use the boolean below to avoid lifetime issues)

[a1phyr]

This condition is wrong: you compare the old buffer capacity and the new buffer spare capacity, but the start of the buffer has changed since then. It would be less error prone to track initialized bytes counting from the beginning of the Vec.

[Darksonn]

Yeah, please store the full capacity of the vector, including anything already written.

[Darksonn]

@rustbot author

[asder8215]

@rustbot ready

[Darksonn]

This is still using the wrong condition. Consider this scenario:

1. We set initialized_len = 10 - 5 where capacity=10, length=5.
2. The vector is resized and the capacity is now 15.
3. We read another 5 bytes with is_init=false.
4. The next iteration compares initialized_len with 15-10 which is true, and considers the buffer initialized.

But in this scenario the buffer was resized and not initialized, so this is wrong. It's really important that initialized_len is such that after the vector is resized, the comparison can never return true until you get a read with is_init=true.

Instead of storing capacity() - len() please just store capacity(). That way, if the capacity changes, then the capacity will no longer be the same.

@rustbot author

[asder8215]

Instead of storing capacity() - len() please just store capacity(). That way, if the capacity changes, then the capacity will no longer be the same.

Just to clarify, resizing only occurs potentially in spots where we call small_probe_reads or in the conditional where buf.len() == buf.capacity() and it does a try_reserve? It should still be safe to use buf_unfilled_len = capacity() - len() on the portion of code where we're converting the spare portion of the buffer into a BorrowedBuf/BorrowedCursor as it shouldn't be able to resize this borrowed buffer/slice with read_buf?

[Darksonn]

It's not obvious to me that that's the case. Maybe you are right, but I can't easily follow the logic.

To me, it would be a lot easier to figure out that the code is correct if you stored the capacity. If the vector was reallocated, then I know that the capacity changed, and therefore I know that we will not call set_init().

[asder8215]

It's not obvious to me that that's the case. Maybe you are right, but I can't easily follow the logic.

I think it does because it seems like the read_buf on BorrowedCursor calls on the default_read_buf, which does the following:

rust/library/std/src/io/mod.rs

Lines 574 to 581 in 36714a9

	pub(crate) fn default_read_buf<F>(read: F, mut cursor: BorrowedCursor<'_, u8>) -> Result<()>
	where
	F: FnOnce(&mut [u8]) -> Result<usize>,
	{
	let n = read(cursor.ensure_init())?;
	cursor.advance_checked(n);
	Ok(())
	}

If I'm understanding it correctly, the advance_checked call here would panic if we try to read more bytes into the unfilled portion of the buffer than what the buffer can handle.

The reason why I asked if it was safe to do buf_unfilled_len = capacity() - len() (in line 476) because that solves the short read issue mentioned in #158008 since from a previous loop it's clear that nothing has been read into the buffer if we have the same unfilled portion length (and that no resizing occurred from earlier small_probe_reads/try_reserve). If I were to do buf_unfilled_len = capacity() at that point there, we're back to the problem that issue had.

[Darksonn]

I'm sorry it looks like you're right. I misunderstood the max_read_size logic.

However, I don't think the current logic looks ideal. Let's say that we have a buffer of capacity 1000 and max_read_size is 100, and that every single read is 1 byte long. Then we get this scenario:

1. Call read(&mut buf[0..100]). After this call, first 100 bytes are initialized.
2. Call read(&mut buf[1..101]). Before this call, the 101th byte is not initialized, so set_init() is not called. After this call, first 101 bytes are initialized.
3. Call read(&mut buf[2..102]). Before this call, the 102th byte is not initialized, so set_init() is not called. After this call, first 102 bytes are initialized.
4. and so on

So it seems like that on short reads, we should repeatedly call read with the same end-index. I.e. we should be calling read(&mut buf[0..100]), then read(&mut buf[1..100]), then read(&mut buf[2..100]) even though this decreases the buffer size each time. That way, the buffer stays initialized between each call.

[asder8215]

@rustbot ready

[Darksonn]

I think it would be less confusing to also set is_init to false here.

View changes since the review

[Darksonn]

This variable keeps track of what operations we have done in the past, but I think it's generally easier to think about variables that keep track of facts about the world.

So for example, could we store the length of the subset of the buffer that we are currently reading into? Before each iteration you do if buf_len > spare.len() { buf_len = spare.len(); }. And after each iteration you do buf_len -= bytes_read followed by if buf_len == 0 { buf_len = max_read_size; }. The is_init variable then keeps track of whether the current subset is initialized or not.

View changes since the review

[asder8215]

I ended up refactoring and using the is_init variable and spare buffer length to determine whether we need to initialize more bytes into the spare buffer or not.

If our spare buffer has a remainder from the modulo operation with max_read_size, it should mean that there are bytes there that we have initialized; otherwise, if we see a 0, then that indicates that we need to initialize more bytes into the spare buffer (aside from the case when our spare buffer is the minimum between itself and the max_read_size in which case taking the mod of that with itself would always produce 0 and anyways we just need to present that remaining spare buffer length).

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[Darksonn]

I believe you have to set is_init to false here too? Since this may increase the number of bytes passed in the next iteration.

View changes since the review

[asder8215]

Doesn't is_init get set to false in the next loop? I believe this conditional doubles max_read_size when it notices that we filled the spare buffer with max_read_size bytes in one iteration rather than through multiple iterations. From the next loop, I think the buf.len() == buf.capacity() conditional earlier should be triggered, resize the buffer, and set is_init to false.

Also, now that I realize this, I think it would be impossible for buf_len > max_read_size because we already accounted for re-using any remaining uninitialized space in the spare buffer rather than creating more uninitialized bytes. I could change this conditional to just bytes_read == max_read_size?

[asder8215]

This conditional right here (which is effectively buf.len() == start_cap due to transitivity) only seems to be ran once to check if the buffer's length is the same as the starting buffer capacity (the ideal case here is that small_probe_read returns 0 and hence we can say for sure that whatever we read into the buffer was an exact fit + avoided doubling the buffer's capacity). Otherwise, if there were more bytes to read, then our buffer's capacity grows and it should be impossible for buf.len() == start_cap (start_cap does not change); therefore, I took this out of the loop since we don't need to run through this multiple times.

View changes since the review

[asder8215]

Still need was_init to set BorrowedBuf init field to true, and I use it down below so that we're not repeatedly assigning init_until with the same value unless we reach a point where we need to initialize more bytes into the spare buffer.

View changes since the review

[Darksonn]

I think this needs to be if is_init { ... } instead. Right now you might update init_until even if is_init == false.

View changes since the review

[asder8215]

Right now you might update init_until even if is_init == false.

Would it be okay to do !was_init && is_init here? This would allow us to avoid repeatedly assigning init_until when we know that it was initialized from a previous loop

[Darksonn]

I guess it's okay, but I don't really see the advantage.

[asder8215]

It probably isn't visible performance-wise. When I was thinking about what init_until was reassigning itself to mentally, I noticed that init_until was updating itself to the same value on the number of initialized bytes in the buffer during short reads.

The was_init variable from earlier keeps track if we had an initialized buffer from previous loop. I just thought it would be cheaper to check through booleans before triggering a reassignment on init_until with usize being 4/8 bytes.

[Darksonn]

Well, performance-wise here is my take:

I would expect if is_init {} to be easier for the branch predictor to predict since most IO resources generally always return false or always return true. Cases where the IO resource dynamically does on or the other are pretty rare. Branches are much more expensive than a single addition.

Also, using was_init here means its value probably has to be stored on the stack while read_buf is invoked, whereas if you don't use it here, it can be discarded before invoking read_buf, meaning it may be able to just store the boolean in a register.

Anyways. Ultimately all of these are just guesses without measuring performance or looking at assembly, and in practice I don't think it matters.

[Darksonn]

Why was this moved outside of the loop? I think it needs to be inside the loop because buf.len() will usually be zero here, so it will basically never trigger.

View changes since the review

[asder8215]

I thought the code above this will call on small_probe_read, so it would be possible for buf.len() to be equal to start_cap?

I didn't see a reason for this needed to remain in the loop since this was a conditional that triggers once on buf.len() == start_cap. The moment we expand the buffer above that start_cap, there shouldn't be a way for that conditional to be triggered. It doesn't seem like we update start_cap either.

[Darksonn]

My understanding is that the purpose of this code is to handle the case where you are reading a, for example 10 MB file, and so you allocate exactly 10 MB, and read those into the buffer over a couple of iterations. Eventually when you have read the file, you want to avoid resizing the buffer when it's not needed, and so the small probe read allows you to check whether the buffer had exactly the right size without resizing it.

[asder8215]

That makes sense to me. I forgot to account for that we could be reading those bytes in and reaching the start_cap over multiple iterations.

I guess in that case would it be beneficial to do a small_probe_read every time we reach buf.len() == buf.capacity()? It can serve as both a way to prevent re-allocating the buffer if we know our read returns 0, but also it should re-allocate the buffer for us in case there are more bytes to read.

Actually, on second thought, that doesn't sound like a good a idea since it expects people to determine how the buffer resize and the capacity it grows to and make sure to read in that many bytes (we maybe have some unnecessary reads). I'll revert to the original

[Darksonn]

Performing a small probe read only makes sense if you think there is a good chance you have hit the size exactly correctly, down to the byte. Outside of the start_cap case, that's pretty unlikely.