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 RawVec::reserve_in_place. #72417

Merged
merged 4 commits into from
Jun 10, 2020

Conversation

nnethercote
Copy link
Contributor

And some related clean-ups.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 21, 2020
@nnethercote
Copy link
Contributor Author

I recommend reviewing the commits one at a time.

This shouldn't affect perf, but I've seen small changes to RawVec::grow have surprisingly large effects, so let's do a perf run to check:
@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented May 21, 2020

⌛ Trying commit c6ac55dcbb3bc9a462147f4bd3530987b93c23fa with merge ce50223feb6be16fc4e3597602fb0326a1e89e3a...

@nnethercote
Copy link
Contributor Author

@bors try

@bors
Copy link
Contributor

bors commented May 21, 2020

⌛ Trying commit 6d514d1fe16b04ec3264c7bd3e396dda4d3c9bdd with merge 8d66106238885e2ad4a8da3600a2dd845483f66f...

@bors
Copy link
Contributor

bors commented May 21, 2020

☀️ Try build successful - checks-azure
Build commit: 8d66106238885e2ad4a8da3600a2dd845483f66f (8d66106238885e2ad4a8da3600a2dd845483f66f)

@rust-timer
Copy link
Collaborator

Queued 8d66106238885e2ad4a8da3600a2dd845483f66f with parent 963bf52, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 8d66106238885e2ad4a8da3600a2dd845483f66f, comparison URL.

@nnethercote
Copy link
Contributor Author

Perf results show a very slight win.

src/libarena/lib.rs Outdated Show resolved Hide resolved
@nnethercote
Copy link
Contributor Author

nnethercote commented May 25, 2020

@Amanieu: with RawVec::reserve_in_place gone, there are no useful uses of ReallocPlacement::InPlace remaining in the codebase. It's only used in AllocRef::grow and AllocRef::shrink, and all implementations of those functions (the default impls in AllocRef, plus the impls in System and Global) simply return an error if InPlace is specified, even for shrink.

Perhaps ReallocPlacement should just be removed? In general InPlace only seems useful if you don't have a way of finding out the actual size of an allocation, in which case you might see if reallocing in place works. But AllocRef does provide a way to get the actual size of an allocation (in theory, if not in current practice), and being able to get the actual size is more useful in general.

@Amanieu
Copy link
Member

Amanieu commented May 25, 2020

reserve_in_place is used in src/libarena.

@Amanieu
Copy link
Member

Amanieu commented May 25, 2020

Oh nevermind, I see you just removes those uses.

@Amanieu
Copy link
Member

Amanieu commented May 25, 2020

cc @TimDiekmann

@TimDiekmann
Copy link
Member

I don't like the idea to remove the feature to grow/shrink in-place from AllocRef. Actually I think this API is missing on Vec, too but wasn't implemented yet as GlobalAlloc does neither support growing in-place, nor overallocating. However ReallocPlacement is pretty long at typing and the order of the parameters in grow has to be looked up, which is why I don't like ReallocPlacement and it's siblings (anymore). There is a proposal around to revert it back do dedicated methods again: rust-lang/wg-allocators#58.

If the API is not going to be added to Vec it's possible to move in-place variants to a third-party crate, but this should be discussed in the linked issue.

@oli-obk oli-obk added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label May 25, 2020
@nnethercote
Copy link
Contributor Author

8 days without review from @oli-obk, let's try someone else:

r? @Amanieu

@Amanieu
Copy link
Member

Amanieu commented May 29, 2020

Blocked on rust-lang/wg-allocators#58

@nnethercote
Copy link
Contributor Author

Blocked on rust-lang/wg-allocators#58

It's not blocked... this PR removes a use of InPlace that was separate to the uses by AllocRef and its impls. It can land right now.

@Amanieu
Copy link
Member

Amanieu commented May 30, 2020

Oh right, good point.

@bors r+

@bors
Copy link
Contributor

bors commented May 30, 2020

📌 Commit 6d514d1fe16b04ec3264c7bd3e396dda4d3c9bdd has been approved by Amanieu

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 2, 2020
…ace, r=Amanieu

Remove `RawVec::reserve_in_place`.

And some related clean-ups.

r? @oli-obk
@JohnTitor
Copy link
Member

This will break rls and rustfmt builds so @bors r- while we're on the beta week.

   Compiling rustc-ap-arena v659.0.0
error[E0599]: no method named `reserve_in_place` found for struct `alloc::raw_vec::RawVec<T>` in the current scope
   --> /cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-arena-659.0.0/lib.rs:228:39
    |
228 |                 if last_chunk.storage.reserve_in_place(currently_used_cap, n) {
    |                                       ^^^^^^^^^^^^^^^^ method not found in `alloc::raw_vec::RawVec<T>`

error[E0599]: no method named `reserve_in_place` found for struct `alloc::raw_vec::RawVec<u8>` in the current scope
   --> /cargo/registry/src/github.com-1ecc6299db9ec823/rustc-ap-arena-659.0.0/lib.rs:356:39
    |
356 |                 if last_chunk.storage.reserve_in_place(used_bytes, needed_bytes) {
    |                                       ^^^^^^^^^^^^^^^^ method not found in `alloc::raw_vec::RawVec<u8>`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0599`.
[RUSTC-TIMING] rustc_ap_arena test:false 0.187
error: could not compile `rustc-ap-arena`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
[RUSTC-TIMING] jsonrpc_server_utils test:false 7.894
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/rls/Cargo.toml" "--features" "clippy, rustc-workspace-hack/all-static" "--message-format" "json-render-diagnostics"
expected success, got: exit code: 101
[TIMING] ToolBuild { compiler: Compiler { stage: 2, host: "x86_64-unknown-linux-gnu" }, target: "x86_64-unknown-linux-gnu", tool: "rls", path: "src/tools/rls", mode: ToolRustc, is_optional_tool: true, source_type: Submodule, extra_features: ["clippy"] } -- 565.495
failed to test rls: could not build

...

error: Tool `rls` has regressed from test-pass to build-fail during beta week.
error: Tool `rustfmt` has regressed from test-pass to build-fail during beta week.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 2, 2020
@bors
Copy link
Contributor

bors commented Jun 6, 2020

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

This causes unnecessary calls to `grow` when the allocation would fit
exactly in the remaining space.
They are pointless. No reasonable allocator will be able to satisfy a
`reserve_in_place` request that *doubles* the size of an allocation when
dealing with allocations that are 4 KiB and larger.

Just to be sure, I confirmed on Linux that the `reserve_in_place` calls
never succeed.

(Note however that the `reserve_in_place` call for `DroplessArena::grow`
did occasionally succeed prior to the off-by-one fix in the previous
commit, because we would sometimes do a `reserve_in_place` request for
the chunk's current size, which would trivially succeed!)
Also remove a now-unnecessary `placement` argument.
- Use `len` more consistently for the number of elements in a vector,
  because that's the usual name.
- Use `additional` more consistently for the number of elements we want
  to add, because that's what `Vec::reserve()` uses.
- Use `cap` consistently rather than `capacity`.
- Plus a few other tweaks.

This increases consistency and conciseness.
@nnethercote
Copy link
Contributor Author

I have rebased and beta week is over.

@bors r=Amanieu

@bors
Copy link
Contributor

bors commented Jun 8, 2020

📌 Commit c9cbe7e has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 8, 2020
@bors
Copy link
Contributor

bors commented Jun 9, 2020

⌛ Testing commit c9cbe7e with merge f8ed3f6c733f2119bda67b3cb01575c03005453b...

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 9, 2020
…ace, r=Amanieu

Remove `RawVec::reserve_in_place`.

And some related clean-ups.

r? @oli-obk
@Dylan-DPC-zz
Copy link

@bors retry (this pr is included in the rollup)

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 10, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#72417 (Remove `RawVec::reserve_in_place`.)
 - rust-lang#73098 (Add Item::is_fake for rustdoc)
 - rust-lang#73122 (Resolve E0584 conflict)
 - rust-lang#73123 (Clean up E0647 explanation)
 - rust-lang#73133 (Enforce unwind invariants)
 - rust-lang#73148 (Fix a typo (size of the size))
 - rust-lang#73149 (typo: awailable -> available)
 - rust-lang#73161 (Add mailmap entry)

Failed merges:

r? @ghost
@bors bors merged commit 860e6bd into rust-lang:master Jun 10, 2020
@nnethercote nnethercote deleted the rm-RawVec-reserve_in_place branch June 10, 2020 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants