Remove initialized-bytes tracking from BorrowedBuf and BorrowedCursor
#148937
Conversation
rustbot
added
S-waiting-on-review
joshtriplett
force-pushed
the
borrowed-buf-no-init-tracking
branch
from
7de0924 to
d131ff2
Compare
joshtriplett
added
T-libs-api
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
| /// `BorrowedCursor`. The cursor has write-only access to the unfilled portion of the buffer. | ||
| /// | ||
| /// The lifetime `'data` is a bound on the lifetime of the underlying data. | ||
| pub struct BorrowedBuf<'data> { |
There was a problem hiding this comment.
Like we previously discussed, this should be made generic over T. For now it's fine to limit it to Copy types, but we may want to relax this later.
There was a problem hiding this comment.
@Amanieu My intention was to do that in a separate PR, to make this PR more manageable to review.
This is a great catch, thank you. I would be inclined to remove the support for making a And removing the support for |
|
We need support for constructing from a |
Wouldn't that still require effectively all of the initialized-bytes tracking we currently have? Or did you have some other strategy in mind here? Would it suffice to drop the change making (That does still seem unfortunate, though, as it'd be that much harder to use this API from safe code.) |
|
|
I see. Then I'll remove the |
…sor` As discussed extensively in libs-api, the initialized-bytes tracking primarily benefits calls to `read_buf` that end up initializing the buffer and calling `read`, at the expense of calls to `read_buf` that *don't* need to initialize the buffer. Essentially, this optimizes for the past at the expense of the future. If people observe performance issues using `read_buf` (or something that calls it) with a given `Read` impl, they can fix those performance issues by implementing `read_buf` for that `Read`. Update the documentation to stop talking about initialized-but-unfilled bytes. Remove all functions that just deal with those bytes and their tracking, and remove usage of those methods. Remove `BorrowedCursor::advance` as there's no longer a safe case for advancing within initialized-but-unfilled bytes. Rename `BorrowedCursor::advance_unchecked` to `advance`. Update tests.
joshtriplett
force-pushed
the
borrowed-buf-no-init-tracking
branch
from
d131ff2 to
3825099
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors r+ |
bors
added
S-waiting-on-bors
Rollup of 3 pull requests Successful merges: - #148937 (Remove initialized-bytes tracking from `BorrowedBuf` and `BorrowedCursor`) - #149553 (added default_uwtables=true to aarch64_unknown_none targets) - #149578 (rustdoc: Fix broken link to `Itertools::format`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 3 pull requests Successful merges: - #148937 (Remove initialized-bytes tracking from `BorrowedBuf` and `BorrowedCursor`) - #149553 (added default_uwtables=true to aarch64_unknown_none targets) - #149578 (rustdoc: Fix broken link to `Itertools::format`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #148937 - joshtriplett:borrowed-buf-no-init-tracking, r=Amanieu Remove initialized-bytes tracking from `BorrowedBuf` and `BorrowedCursor` As discussed extensively in libs-api, the initialized-bytes tracking primarily benefits calls to `read_buf` that end up initializing the buffer and calling `read`, at the expense of calls to `read_buf` that *don't* need to initialize the buffer. Essentially, this optimizes for the past at the expense of the future. If people observe performance issues using `read_buf` (or something that calls it) with a given `Read` impl, they can fix those performance issues by implementing `read_buf` for that `Read`. Update the documentation to stop talking about initialized-but-unfilled bytes. Remove all functions that just deal with those bytes and their tracking, and remove usage of those methods. Remove `BorrowedCursor::advance` as there's no longer a safe case for advancing within initialized-but-unfilled bytes. Rename `BorrowedCursor::advance_unchecked` to `advance`. Update tests. r? ``@Amanieu``
| /// has write-only access to the unfilled portion of the buffer (you can think of it as a | ||
| /// write-only iterator). | ||
| /// Note that `BorrowedBuf` does not distinguish between uninitialized data and data that was | ||
| /// previously initialized but no longer contains valid data. |
There was a problem hiding this comment.
"data... that no longer contains valid data" sounds a bit odd?
7 participants
As discussed extensively in libs-api, the initialized-bytes tracking primarily benefits calls to
read_bufthat end up initializing the buffer and callingread, at the expense of calls toread_bufthat don't need to initialize the buffer. Essentially, this optimizes for the past at the expense of the future. If people observe performance issues usingread_buf(or something that calls it) with a givenReadimpl, they can fix those performance issues by implementingread_buffor thatRead.Update the documentation to stop talking about initialized-but-unfilled bytes.
Remove all functions that just deal with those bytes and their tracking, and remove usage of those methods.
Remove
BorrowedCursor::advanceas there's no longer a safe case for advancing within initialized-but-unfilled bytes. RenameBorrowedCursor::advance_uncheckedtoadvance.Update tests.
r? @Amanieu