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

Remove redundant reserve call #674

Merged
merged 4 commits into from Mar 4, 2024

Conversation

braddunbar
Copy link
Contributor

Note: Miri is currently failing in CI, but is fixed by #673

We're calling reserve twice because put_u8 already calls it.

  1. put_u8 calls put_slice
  2. BytesMut::put_slice calls BytesMut::extend_from_slice
  3. BytesMut::extend_from_slice immediately calls reserve

1. put_u8 calls put_slice
2. BytesMut::put_slice calls BytesMut::extend_from_slice
3. BytesMut::extend_from_slice immediately calls reserve
reserve is already inline-able
@@ -1283,9 +1283,7 @@ impl Extend<u8> for BytesMut {

// TODO: optimize
// 1. If self.kind() == KIND_VEC, use Vec::extend
// 2. Make `reserve` inline-able
Copy link
Member

@taiki-e taiki-e Mar 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what this TODO comment originally meant, but if it means something like "reserve (in here or put_u8) should be able to omit if the compiler knows the actual length of iter" it may not have been resolved yet.

(In my experience, the compiler optimizations around the reserve have not worked that well.)

EDIT: but seanmonstar who wrote this TODO comment approved this PR, so this is probably not the meaning I have imagined here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @taiki-e! I think it refers to #313, which was just after the introduction of the comment in #305. I should've mentioned that in my commit though. Thanks for taking a look!

@braddunbar
Copy link
Contributor Author

Updated with #673 to fix miri tests!

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before accepting this PR, I would like to see a test that calls extend with an iterator containing more items than the lower limit of the size hint. I'm worried about the panic conditions of put_u8.

Edit: I guess the behavior is probably okay due to how BytesMut pretends to have an infinite capacity? Still, the test would be good to add to guard against future changes.

@braddunbar
Copy link
Contributor Author

@Darksonn Sure thing! Added in 76dae7b. I think the following test technically covers it via Extend<&'a u8>, but I like being explicit.

@Darksonn Darksonn merged commit 7968f6f into tokio-rs:master Mar 4, 2024
15 checks passed
@braddunbar braddunbar mentioned this pull request Mar 22, 2024
@braddunbar braddunbar deleted the redundant-reserve branch March 24, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants