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

avoid unnecessary reservations in std::io::Take::read_to_end #63216

Open
wants to merge 1 commit into
base: master
from

Conversation

@oconnor663
Copy link
Contributor

commented Aug 2, 2019

Prevously the read_to_end implementation for std::io::Take used its
own limit as a cap on the reservation_size. However, that could
still result in an over-allocation like this:

  1. Call reader.take(5).read_to_end(&mut vec).
  2. read_to_end_with_reservation reserves 5 bytes and calls read.
  3. read writes 5 bytes.
  4. read_to_end_with_reservation reserves 5 bytes and calls read.
  5. read writes 0 bytes.
  6. The read loop ends with vec having length 5 and capacity 10.

The reservation of 5 bytes was correct for the read at step 2 but
unnecessary for the read at step 4. By that second read, Take::limit
is 0, but the read_to_end_with_reservation loop is still using the
same reservation_size it started with.

Solve this by having read_to_end_with_reservation take a closure,
which lets it get a fresh reservation_size for each read. This is an
implementation detail which doesn't affect any public API.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 2, 2019

r? @bluss

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

commented Aug 2, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-02T14:29:42.2977970Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-08-02T14:29:42.3162418Z ##[command]git config gc.auto 0
2019-08-02T14:29:42.3235881Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-08-02T14:29:42.3284672Z ##[command]git config --get-all http.proxy
2019-08-02T14:29:42.3420054Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/63216/merge:refs/remotes/pull/63216/merge
---
2019-08-02T14:30:17.4953393Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-02T14:30:17.4953419Z 
2019-08-02T14:30:17.4953575Z   git checkout -b <new-branch-name>
2019-08-02T14:30:17.4953598Z 
2019-08-02T14:30:17.4953651Z HEAD is now at 214f52274 Merge bf4e73ffe4365cb29041095f0e41c3f9d6fcb20e into fc3ef9698fa80aa2f4da6208b8295bc8fa48eec5
2019-08-02T14:30:17.5096840Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-02T14:30:17.5099753Z ==============================================================================
2019-08-02T14:30:17.5099797Z Task         : Bash
2019-08-02T14:30:17.5099847Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-02T14:36:06.9038673Z    Compiling serde_json v1.0.40
2019-08-02T14:36:10.8165911Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-08-02T14:36:18.5465145Z     Finished release [optimized] target(s) in 1m 20s
2019-08-02T14:36:18.5529298Z tidy check
2019-08-02T14:36:19.0453311Z tidy error: /checkout/src/libstd/io/mod.rs:359: line longer than 100 chars
2019-08-02T14:36:20.3035740Z some tidy checks failed
2019-08-02T14:36:20.3040389Z 
2019-08-02T14:36:20.3040389Z 
2019-08-02T14:36:20.3041694Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-08-02T14:36:20.3042040Z 
2019-08-02T14:36:20.3042062Z 
2019-08-02T14:36:20.3046374Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-08-02T14:36:20.3046499Z Build completed unsuccessfully in 0:01:23
2019-08-02T14:36:20.3046499Z Build completed unsuccessfully in 0:01:23
2019-08-02T14:36:21.7480562Z ##[error]Bash exited with code '1'.
2019-08-02T14:36:21.7508993Z ##[section]Starting: Checkout
2019-08-02T14:36:21.7510348Z ==============================================================================
2019-08-02T14:36:21.7510390Z Task         : Get sources
2019-08-02T14:36:21.7510426Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@oconnor663 oconnor663 force-pushed the oconnor663:take_read_to_end branch from bf4e73f to 5b4b937 Aug 2, 2019

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

☔️ The latest upstream changes (presumably #62987) made this pull request unmergeable. Please resolve the merge conflicts.

avoid unnecessary reservations in std::io::Take::read_to_end
Prevously the `read_to_end` implementation for `std::io::Take` used its
own `limit` as a cap on the `reservation_size`. However, that could
still result in an over-allocation like this:

1. Call `reader.take(5).read_to_end(&mut vec)`.
2. `read_to_end_with_reservation` reserves 5 bytes and calls `read`.
3. `read` writes 5 bytes.
4. `read_to_end_with_reservation` reserves 5 bytes and calls `read`.
5. `read` writes 0 bytes.
6. The read loop ends with `vec` having length 5 and capacity 10.

The reservation of 5 bytes was correct for the read at step 2 but
unnecessary for the read at step 4. By that second read, `Take::limit`
is 0, but the `read_to_end_with_reservation` loop is still using the
same `reservation_size` it started with.

Solve this by having `read_to_end_with_reservation` take a closure,
which lets it get a fresh `reservation_size` for each read. This is an
implementation detail which doesn't affect any public API.

@oconnor663 oconnor663 force-pushed the oconnor663:take_read_to_end branch from 5b4b937 to edb5214 Aug 6, 2019

@oconnor663

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@bluss let me know if I should grab a different reviewer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.