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

Add support for allocators in Rc & Arc #89132

Merged
merged 5 commits into from
Jul 17, 2023

Conversation

Cyborus04
Copy link
Contributor

@Cyborus04 Cyborus04 commented Sep 20, 2021

Adds the ability for std::rc:Rc, std::rc::Weak, std::sync::Arc, and std::sync::Weak to live in custom allocators

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 20, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Cyborus04
Copy link
Contributor Author

Oh, finally

This was a mess, I'm so sorry.

@Cyborus04 Cyborus04 changed the title Add support for allocators in Rc Add support for allocators in Rc Sep 21, 2021
@jyn514
Copy link
Member

jyn514 commented Sep 22, 2021

@Cyborus04 can you squash the commits? 35 is a bit much haha

@Cyborus04
Copy link
Contributor Author

Oh dang, was it really that many? Haha wow, sorry
5 seems better

@jyn514 jyn514 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-allocators Area: Custom and system allocators labels Sep 25, 2021
@bors
Copy link
Contributor

bors commented Sep 26, 2021

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

@Cyborus04
Copy link
Contributor Author

I think I did that right? The guide is for the Git CLI but I used Github Desktop so I'm not entirely sure

@bors
Copy link
Contributor

bors commented Sep 30, 2021

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

@Cyborus04
Copy link
Contributor Author

Cyborus04 commented Sep 30, 2021

I'm so confused, I didn't think I touched those files.
(src/tools/cargo & src/tools/rust-analyzer)

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jul 3, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 3, 2023
@tgross35
Copy link
Contributor

tgross35 commented Jul 3, 2023

This test that failed has the output:

2023-07-03T20:29:37.2300246Z ---- [debuginfo-cdb] tests\debuginfo\rc_arc.rs stdout ----
2023-07-03T20:29:37.2300549Z 
2023-07-03T20:29:37.2308293Z error: line not found in debugger output: slice_rc,d       : { len=3 } [Type: alloc::rc::Rc<slice2$<u32>,alloc::alloc::Global>]
2023-07-03T20:29:37.2308643Z status: exit code: 0
...
2023-07-03T20:29:37.2318224Z --- stdout -------------------------------
...
2023-07-03T20:29:37.2355587Z slice_rc,d       [Type: alloc::rc::Rc<slice2$<u32>,alloc::alloc::Global>]
2023-07-03T20:29:37.2355883Z     [<Raw View>]     [Type: alloc::rc::Rc<slice2$<u32>,alloc::alloc::Global>]
2023-07-03T20:29:37.2356162Z 0:000> dx slice_rc_weak,d
2023-07-03T20:29:37.2356409Z slice_rc_weak,d  [Type: alloc::rc::Weak<slice2$<u32>,alloc::alloc::Global>]
2023-07-03T20:29:37.2356761Z Build completed unsuccessfully in 0:37:29
...
2023-07-03T20:29:37.2366562Z ------------------------------------------
2023-07-03T20:29:37.2366755Z stderr: none

It doesn't seem like this can be right... Isn't that exact text right there? I think we need a hand understanding what is going on, if anyone is more familiar with these tests

@Cyborus04
Copy link
Contributor Author

Pointed out in Zulip that the { len=3 } is missing in the debugger, but if that's the error, why didn't Arc have an issue too?

@tgross35
Copy link
Contributor

tgross35 commented Jul 6, 2023

It looks like these strings are generated here:

<!--
The display string for Rc, Arc, etc is optional because the expression cannot be evaluated
if the pointee is unsized (i.e. if `ptr.pointer` is a fat pointer).
There are also two versions for the reference count fields, one for sized and one for
dyn pointees.
Rc<[T]> and Arc<[T]> are handled separately altogether so we can actually show
the slice values.
-->
<!-- alloc::rc::Rc<T> -->
<Type Name="alloc::rc::Rc&lt;*&gt;">
<DisplayString Optional="true">{ptr.pointer->value}</DisplayString>
<Expand>
<!-- thin -->
<ExpandedItem Optional="true">ptr.pointer->value</ExpandedItem>
<Item Name="[Reference count]" Optional="true">ptr.pointer->strong</Item>
<Item Name="[Weak reference count]" Optional="true">ptr.pointer->weak</Item>
<!-- dyn -->
<Item Name="[Reference count]" Optional="true">ptr.pointer.pointer->strong</Item>
<Item Name="[Weak reference count]" Optional="true">ptr.pointer.pointer->weak</Item>
</Expand>
</Type>
<!-- alloc::rc::Rc<[T]> -->
<Type Name="alloc::rc::Rc&lt;slice2$&lt;*&gt; &gt;">
<DisplayString>{{ len={ptr.pointer.length} }}</DisplayString>
<Expand>
<Item Name="[Length]" ExcludeView="simple">ptr.pointer.length</Item>
<Item Name="[Reference count]">ptr.pointer.data_ptr->strong</Item>
<Item Name="[Weak reference count]">ptr.pointer.data_ptr->weak</Item>
<ArrayItems>
<Size>ptr.pointer.length</Size>
<!-- We add +2 to the data_ptr in order to skip the ref count fields in the RcBox -->
<ValuePointer>($T1*)(((size_t*)ptr.pointer.data_ptr) + 2)</ValuePointer>
</ArrayItems>
</Expand>
</Type>
<!-- alloc::rc::Weak<T> -->
<Type Name="alloc::rc::Weak&lt;*&gt;">
<DisplayString Optional="true">{ptr.pointer->value}</DisplayString>
<Expand>
<!-- thin -->
<ExpandedItem Optional="true">ptr.pointer->value</ExpandedItem>
<Item Name="[Reference count]" Optional="true">ptr.pointer->strong</Item>
<Item Name="[Weak reference count]" Optional="true">ptr.pointer->weak</Item>
<!-- dyn -->
<Item Name="[Reference count]" Optional="true">ptr.pointer.pointer->strong</Item>
<Item Name="[Weak reference count]" Optional="true">ptr.pointer.pointer->weak</Item>
</Expand>
</Type>
<!-- alloc::rc::Weak<[T]> -->
<Type Name="alloc::rc::Weak&lt;slice2$&lt;*&gt; &gt;">
<DisplayString>{{ len={ptr.pointer.length} }}</DisplayString>
<Expand>
<Item Name="[Length]" ExcludeView="simple">ptr.pointer.length</Item>
<Item Name="[Reference count]">ptr.pointer.data_ptr->strong</Item>
<Item Name="[Weak reference count]">ptr.pointer.data_ptr->weak</Item>
<ArrayItems>
<Size>ptr.pointer.length</Size>
<ValuePointer>($T1*)(((size_t*)ptr.pointer.data_ptr) + 2)</ValuePointer>
</ArrayItems>
</Expand>
</Type>
<!-- alloc::sync::Arc<T> -->
<Type Name="alloc::sync::Arc&lt;*&gt;">
<DisplayString Optional="true">{ptr.pointer->data}</DisplayString>
<Expand>
<!-- thin -->
<ExpandedItem Optional="true">ptr.pointer->data</ExpandedItem>
<Item Name="[Reference count]" Optional="true">ptr.pointer->strong</Item>
<Item Name="[Weak reference count]" Optional="true">ptr.pointer->weak</Item>
<!-- dyn -->
<Item Name="[Reference count]" Optional="true">ptr.pointer.pointer->strong</Item>
<Item Name="[Weak reference count]" Optional="true">ptr.pointer.pointer->weak</Item>
</Expand>
</Type>
<!-- alloc::sync::Arc<[T]> -->
<Type Name="alloc::sync::Arc&lt;slice2$&lt;*&gt; &gt;">
<DisplayString>{{ len={ptr.pointer.length} }}</DisplayString>
<Expand>
<Item Name="[Length]" ExcludeView="simple">ptr.pointer.length</Item>
<Item Name="[Reference count]">ptr.pointer.data_ptr->strong</Item>
<Item Name="[Weak reference count]">ptr.pointer.data_ptr->weak</Item>
<ArrayItems>
<Size>ptr.pointer.length</Size>
<ValuePointer>($T1*)(((size_t*)ptr.pointer.data_ptr) + 2)</ValuePointer>
</ArrayItems>
</Expand>
</Type>
<!-- alloc::sync::Weak<T> -->
<Type Name="alloc::sync::Weak&lt;*&gt;">
<DisplayString Optional="true">{ptr.pointer->data}</DisplayString>
<Expand>
<!-- thin -->
<ExpandedItem Optional="true">ptr.pointer->data</ExpandedItem>
<Item Name="[Reference count]" Optional="true">ptr.pointer->strong</Item>
<Item Name="[Weak reference count]" Optional="true">ptr.pointer->weak</Item>
<!-- dyn -->
<Item Name="[Reference count]" Optional="true">ptr.pointer.pointer->strong</Item>
<Item Name="[Weak reference count]" Optional="true">ptr.pointer.pointer->weak</Item>
</Expand>
</Type>
<!-- alloc::sync::Weak<[T]> -->
<Type Name="alloc::sync::Weak&lt;slice2$&lt;*&gt; &gt;">
<DisplayString>{{ len={ptr.pointer.length} }}</DisplayString>
<Expand>
<Item Name="[Length]" ExcludeView="simple">ptr.pointer.length</Item>
<Item Name="[Reference count]">ptr.pointer.data_ptr->strong</Item>
<Item Name="[Weak reference count]">ptr.pointer.data_ptr->weak</Item>
<ArrayItems>
<Size>ptr.pointer.length</Size>
<ValuePointer>($T1*)(((size_t*)ptr.pointer.data_ptr) + 2)</ValuePointer>
</ArrayItems>
</Expand>
</Type>
</AutoVisualizer>
but I can't figure out why the length would not be displayed. Unless the ptr.pointer part isn't computing for some reason? I don't think anything there should have changed.

cc @rust-lang/wg-debugging, would you be able to help us figure out what's going on with the mismatched debug output here?

(edit: sorry wg-mir-opt, I have no clue why I accidentally cc'd you instead of wg-debugging)

Also update a test case to have the correct whitespace in a type name.
@wesleywiser
Copy link
Member

Hi @Cyborus04 and @tgross35 👋

I took the liberty of pushing a commit into your branch that fixes the failing debuginfo tests. The main thing to note is that some of the natvis visualizers do a partial match like this Rc<[*]> but with this change, they needed to be adjusted to do Rc<[*], *> instead. With that done, the tests pass locally for me so I think this is ready for another:

@bors r=Amanieu

@bors
Copy link
Contributor

bors commented Jul 17, 2023

📌 Commit 12bed9d has been approved by Amanieu

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2023
@tgross35
Copy link
Contributor

Thank you for taking a look Wesley! Don't think we ever would have figured that out on our own 🙂

@Cyborus04
Copy link
Contributor Author

yes, tysm!

@bors
Copy link
Contributor

bors commented Jul 17, 2023

⌛ Testing commit 12bed9d with merge da6b55c...

@bors
Copy link
Contributor

bors commented Jul 17, 2023

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing da6b55c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 17, 2023
@bors bors merged commit da6b55c into rust-lang:master Jul 17, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Jul 17, 2023
@Cyborus04
Copy link
Contributor Author

I had a double take when I saw the purple icon in my notifications, I'm so happy!

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (da6b55c): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.8% [0.8%, 0.8%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.3% [-1.3%, -1.3%] 1
Improvements ✅
(secondary)
-0.7% [-0.8%, -0.5%] 4
All ❌✅ (primary) -0.3% [-1.3%, 0.8%] 2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-3.5% [-4.5%, -2.5%] 2
Improvements ✅
(secondary)
-2.6% [-2.6%, -2.6%] 1
All ❌✅ (primary) -3.5% [-4.5%, -2.5%] 2

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.9% [1.9%, 1.9%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.9% [1.9%, 1.9%] 1

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.2% [0.0%, 0.8%] 42
Regressions ❌
(secondary)
1.5% [1.5%, 1.5%] 4
Improvements ✅
(primary)
-0.1% [-0.2%, -0.0%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [-0.2%, 0.8%] 45

Bootstrap: 657.068s -> 658.015s (0.14%)

@pnkfelix
Copy link
Member

Visiting for perf-triage

  • primary regression was to image-0.24.1 opt full by 0.79%
  • I think this is just noise. From the graph, it seems like image has unpredictably jumped up and down between two plateaus since PR Eliminate ZST allocations in Box and Vec #113113 (a PR discussed up above that changed low level allocation procotol code in Box and Vec, and thus might be expected to have some weird follow-on effects).
  • marking as triaged

@rustbot label: +perf-regression-triaged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-allocators Area: Custom and system allocators merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.