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

Fix unnecessary zeroing for AsyncRead implementors #2525

Merged
merged 18 commits into from
May 21, 2020

Conversation

MikailBag
Copy link
Contributor

Fixes #2308

for x in buf {
// we could use safe method x.write(0) here, but it is unstable.
// Safety: as_mut_ptr() returns valid writeable pointer.
unsafe {
Copy link
Contributor

Choose a reason for hiding this comment

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

*x = MaybeUninit::new(0);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated both places.

@Darksonn Darksonn added A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-fs Module: tokio/fs M-io Module: tokio/io M-process Module: tokio/process T-performance Topic: performance and benchmarks labels May 12, 2020
@MikailBag
Copy link
Contributor Author

MikailBag commented May 12, 2020

How should I feature-gate prepare_uninitialized_buffer_std_read()?
I don't know which features should be used.

UPD: I fixed this problem by making that function pub. I think it's reasonable, because this function is useful to every type that wraps Read internally.

@MikailBag MikailBag marked this pull request as ready for review May 13, 2020 16:23
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.

This seems reasonable to me.

@Darksonn
Copy link
Contributor

Oh, I didn't see that this exposes prepare_uninitialized_buffer_std_read. Is that really that useful?

@MikailBag
Copy link
Contributor Author

I am not sure it is useful for outside users. I will make it private, then.

Comment on lines 8 to 11
/// Implements `AsyncRead::prepare_uninitialized_buffer` in a such a way
/// that is valid for `R` requirements.
/// Should be used to override `AsyncRead::prepare_uninitialized_buffer`
/// for any AsyncRead impl which wraps an `std::io::Read` instance.
Copy link
Member

Choose a reason for hiding this comment

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

If this is going to be publicly exposed (and I'm not convinced it needs to be), it would be nice to have a more detailed comment explaining what it does and why users would want to use it...

@MikailBag
Copy link
Contributor Author

I made that function private again, but I can't solve #[cfg] problems. These problems are definitely related to tests. I can't use plain cfg-s because tests want to override it. I can't use cfg_* macros as well because we need OR of cfg_io_blocking and cfg_io_driver, and nested macros effect in AND.

@MikailBag
Copy link
Contributor Author

Alternatively, I can remove all feature-gates, and annotate that function with #[allow(dead_code)], but I fear it would be very poor code style.

@Darksonn
Copy link
Contributor

What's the issue with tests? I don't expect tests to successfully run without all features turned on. Anyway, if you can't find another way to fix it, I'm okay with #[allow(dead_code)].

@MikailBag
Copy link
Contributor Author

I expanded prepare_uninitialized_buffer_std_read documentation a bit and annotated with allow(dead_code). I think PR is now finished (except for awful commit history). Can I squash it now?

@Darksonn
Copy link
Contributor

Darksonn commented May 21, 2020

No, please leave the history intact. GitHub will ensure that the commit that ends up on master is properly squashed.

If you want to provide an expanded commit message to the commit on master, you can push an empty commit with the description, and I'll make sure it ends up as the description on the squashed commit.

@MikailBag
Copy link
Contributor Author

I think I finally understood why CI is failing.
test fs_file_mocked mocks all tokio::io APIs.
It include!s src/fs/file.rs and src/io/blocking.rs, and imports all other tokio::io items. However, tokio::io::prepare_uninitialized_buffer_std_read is not imported because it is pub(crate). So, when Blocking implementation is compiled in context of fs_file_mocked test (which has no access to private crate items), it can not find crate::io::prepare_uninitialized_buffer_std_read, and build fails.
I don't have good solution for this problem.
I can mark prepare_uninitialized_buffer_std_read as pub and #[doc(hidden)] with comment discouraging using this function, e.g. "this function is marked public for usage in tests. It is not part of API and should not be used outside of Tokio".
I can confirm that with this change cargo test --all-features succeeds (previsouly fs_file_mocked test would not compile).
I will now commit this change; I hope CI will finally pass :)

@Darksonn
Copy link
Contributor

Maybe just delete the function and copy its body into blocking?

@MikailBag
Copy link
Contributor Author

Thanks, I implemented your approach, it really looks better that doc(hidden) public item.

@MikailBag
Copy link
Contributor Author

MikailBag commented May 21, 2020

I was told in Discord that PollEvented will likely be excluded from Tokio 0.3, and Read::initializer is unlikely to be stabilized before that.
It means all machinery introduced in this PR will not bring any benefits, so I'm going to rollback prepare_uninitialized_buffer_std_read function at all.
Changes to concrete AsyncRead implementors are still in place.

@Darksonn Darksonn merged commit 1e54a35 into tokio-rs:master May 21, 2020
@MikailBag MikailBag deleted the async-read-uninit branch May 21, 2020 18:21
jensim pushed a commit to jensim/tokio that referenced this pull request Jun 7, 2020
hawkw added a commit that referenced this pull request Jul 20, 2020
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2572, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)

- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- io: fix panic in `AsyncReadExt::read_line` (#2541)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit that referenced this pull request Jul 22, 2020
# 0.2.22 (July 2!, 2020)

### Fixes
- docs: misc improvements (#2572, #2658, #2663, #2656, #2647, #2630, #2487, #2621,
  #2624, #2600, #2623, #2622, #2577, #2569, #2589, #2575, #2540, #2564, #2567,
  #2520, #2521, #2493)
- rt: allow calls to `block_on` inside calls to `block_in_place` that are
  themselves inside `block_on` (#2645)
- net: fix non-portable behavior when dropping `TcpStream` `OwnedWriteHalf` (#2597)
- io: improve stack usage by allocating large buffers on directly on the heap
  (#2634)
- io: fix unsound pin projection in `AsyncReadExt::read_buf` and
  `AsyncWriteExt::write_buf` (#2612)
- io: fix unnecessary zeroing for `AsyncRead` implementors (#2525)
- io: Fix `BufReader` not correctly forwarding `poll_write_buf` (#2654)
- io: fix panic in `AsyncReadExt::read_line` (#2541)

### Changes
- coop: returning `Poll::Pending` no longer decrements the task budget (#2549)

### Added
- io: little-endian variants of `AsyncReadExt` and `AsyncWriteExt` methods
  (#1915)
- task: add [`tracing`] instrumentation to spawned tasks (#2655)
- sync: allow unsized types in `Mutex` and `RwLock` (via `default` constructors)
  (#2615)
- net: add `ToSocketAddrs` implementation for `&[SocketAddr]` (#2604)
- fs: add `OpenOptionsExt` for `OpenOptions` (#2515)
- fs: add `DirBuilder` (#2524)

[`tracing`]: https://crates.io/crates/tracing

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-enhancement Category: A PR with an enhancement or bugfix. M-fs Module: tokio/fs M-io Module: tokio/io M-process Module: tokio/process T-performance Topic: performance and benchmarks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Several AsyncRead impls unnecessarily zero buffers in prepare_uninitialized_buffer
4 participants