Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor Rc and Arc to use a prefix allocator #84338

Closed
wants to merge 22 commits into from

Conversation

TimDiekmann
Copy link
Member

@TimDiekmann TimDiekmann commented Apr 19, 2021

This picks up #80273 and splits the inner part of (A)Rc into two. Both structs are now allocated as if they were allocated with a prefix allocator, which may be useful in the future. For now, the allocator is gated behind allocator_api_internals.

The Tracker-allocator is used in tests to ensure that the safety rules are not violated.

r? @Amanieu
This requires a perf-run.

Update: 2021/04/22: With this, Rc and Arc now stores the pointer returned from the allocator, i.e. it points to T directly because accessing the data in smart pointers is more common than copying cloning the smart pointer. The layout of the allocation didn't change.

@rust-log-analyzer

This comment has been minimized.

@TimDiekmann TimDiekmann marked this pull request as draft April 19, 2021 19:39
@jyn514
Copy link
Member

jyn514 commented Apr 20, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Apr 20, 2021
@bors
Copy link
Collaborator

bors commented Apr 20, 2021

⌛ Trying commit fba9038 with merge 5ad6f8abe444911218eb868d3f12eb00538e1c3d...

@bors
Copy link
Collaborator

bors commented Apr 20, 2021

☀️ Try build successful - checks-actions
Build commit: 5ad6f8abe444911218eb868d3f12eb00538e1c3d (5ad6f8abe444911218eb868d3f12eb00538e1c3d)

@rust-timer
Copy link
Collaborator

Queued 5ad6f8abe444911218eb868d3f12eb00538e1c3d with parent 7d0132a, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (5ad6f8abe444911218eb868d3f12eb00538e1c3d): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Apr 20, 2021
@rust-log-analyzer

This comment has been minimized.

@TimDiekmann

This comment has been minimized.

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2021
@TimDiekmann TimDiekmann changed the title Restructure Rc and Arc metadata for uniform access via a PrefixAlloc Refactor Rc and Arc to use a prefix allocator Apr 21, 2021
@rust-log-analyzer

This comment has been minimized.

Rc::drop uses this when strong count is zero
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 24, 2021
@bors
Copy link
Collaborator

bors commented May 24, 2021

⌛ Trying commit 7944de5 with merge 609c7190b20ab83f97550c10f0e98b03cb3b80bc...

@bors
Copy link
Collaborator

bors commented May 24, 2021

☀️ Try build successful - checks-actions
Build commit: 609c7190b20ab83f97550c10f0e98b03cb3b80bc (609c7190b20ab83f97550c10f0e98b03cb3b80bc)

@rust-timer
Copy link
Collaborator

Queued 609c7190b20ab83f97550c10f0e98b03cb3b80bc with parent bf24e6b, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (609c7190b20ab83f97550c10f0e98b03cb3b80bc): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 24, 2021
@TimDiekmann
Copy link
Member Author

The main regression comes from the optimizing path in LLVM (LLVM_module_optimize_module_passes and LLVM_lto_optimize). I was able to help the compiler out and simplify the critical sections. I'll push another commit later.

$ cargo +stage1 llvm-lines -q --release 2> /dev/null | grep 'alloc::' | head -20
   2256 (3.3%)    34 (1.1%)  alloc::alloc::box_free
   1856 (2.7%)    39 (1.3%)  alloc::rc::Rc<T>::new
   1505 (2.2%)    39 (1.3%)  <alloc::rc::Rc<T> as core::ops::drop::Drop>::drop
   1482 (2.2%)    26 (0.8%)  alloc::raw_vec::RawVec<T,A>::current_memory
    984 (1.4%)    41 (1.3%)  core::alloc::layout::Layout::for_value_raw
    952 (1.4%)     7 (0.2%)  alloc::raw_vec::RawVec<T,A>::grow_amortized
    832 (1.2%)    52 (1.7%)  core::alloc::layout::size_align
    810 (1.2%)     9 (0.3%)  core::alloc::layout::Layout::array
    806 (1.2%)    26 (0.8%)  <alloc::raw_vec::RawVec<T,A> as core::ops::drop::Drop>::drop
    710 (1.0%)     5 (0.2%)  alloc::raw_vec::RawVec<T,A>::allocate_in
    616 (0.9%)     5 (0.2%)  <T as alloc::slice::hack::ConvertVec>::to_vec
    572 (0.8%)    52 (1.7%)  core::alloc::layout::Layout::new
    468 (0.7%)    39 (1.3%)  alloc::rc::Rc<T>::metadata
    468 (0.7%)    20 (0.6%)  <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
    351 (0.5%)    39 (1.3%)  alloc::rc::Rc<T>::metadata_ptr
    342 (0.5%)     6 (0.2%)  alloc::vec::Vec<T,A>::push
    280 (0.4%)    28 (0.9%)  <alloc::rc::Rc<T> as core::clone::Clone>::clone
    275 (0.4%)    26 (0.8%)  <alloc::vec::Vec<T,A> as core::ops::drop::Drop>::drop
    234 (0.3%)    39 (1.3%)  alloc::rc::Rc<T>::from_non_null
    234 (0.3%)    26 (0.8%)  alloc::vec::Vec<T,A>::as_mut_ptr

Lines: 68053, Copies: 3117

@Amanieu
Copy link
Member

Amanieu commented May 27, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 27, 2021
@bors
Copy link
Collaborator

bors commented May 27, 2021

⌛ Trying commit a4b4fe4 with merge 205b6d4448728ebbc7f6414144db71d394ca9cb2...

@bors
Copy link
Collaborator

bors commented May 27, 2021

☀️ Try build successful - checks-actions
Build commit: 205b6d4448728ebbc7f6414144db71d394ca9cb2 (205b6d4448728ebbc7f6414144db71d394ca9cb2)

@rust-timer
Copy link
Collaborator

Queued 205b6d4448728ebbc7f6414144db71d394ca9cb2 with parent 86ac0b4, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (205b6d4448728ebbc7f6414144db71d394ca9cb2): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 27, 2021
@TimDiekmann

This comment has been minimized.

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 27, 2021
@Amanieu
Copy link
Member

Amanieu commented May 27, 2021

This is still a pretty significant compilation time regression. I don't think we can land this as it is.

@JohnCSimon
Copy link
Member

Ping from triage:
@TimDiekmann moving this back to author

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2021
@crlf0710
Copy link
Member

crlf0710 commented Jul 4, 2021

@TimDiekmann Ping from triage, any updates on this?

@TimDiekmann
Copy link
Member Author

Sadly I wasn't able to make further optimizations, so I close that for now.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants