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.

Sign up for GitHub

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

Jump to bottom

Inline Box drop glue #46638

Closed
wants to merge 1 commit into from
Closed

Inline Box drop glue #46638

wants to merge 1 commit into from

Conversation

RalfJung
Copy link
Member

Box<T> drop glue is pretty simple and should not lead to blow-up problems.
Moreover, not inlining it leads to LLVM pessimizing some pointer-based
optimizations because it thinks the pointer passed to the drop glue could be
escaped.

Fixes #46515. However, this is not a great fix -- the problem still exists for
other smart pointers, e.g. Rc and Arc.

Cc @arielb1 who wrote the "noinline drop glue" patch (#42771).

@RalfJung
Inline Box drop glue
34cc5c8 
Box<T> drop glue is pretty simple and should not lead to blow-up problems.
Moreover, not inlining it leads to LLVM pessimizing some pointer-based
optimizations because it thinks the pointer passed to the drop glue could be
escaped.

Fixes rust-lang#46515.  However, this is not a great fix -- the problem still exists for
other smart pointers besides `Box`.
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

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

@RalfJung
Copy link
Member Author

Also, I'd like to add a codegen test for this -- how would I go about that?

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 10, 2017
@kennytm
Copy link
Member

kennytm commented Dec 10, 2017

Have you verified if this will regress in #41696?

@RalfJung
Copy link
Member Author

RalfJung commented Dec 10, 2017 •
edited
Loading

I tried the test at #41696 (comment), and it still completes pretty much instantaneously even with opt-level=3 (which nowadays seems to be needed to trigger the bad include).

Some codegen tests fail with this PR; I don't understand their syntax and what exactly they are checking very well.

@hanna-kruppe
Copy link
Contributor

It's unsurprising that this doesn't impact the test case from #41696 (comment) since that code doesn't involve Box at all. However, IIUC the same issue should apply to a small modification to introduce a Box (e.g., by boxing the next field) and that variant will be affected by this PR.

@arielb1
Copy link
Contributor

arielb1 commented Dec 12, 2017 •
edited
Loading

@RalfJung

This PR only improves the situation with Box<Foo> - it does not help with e.g. Vec<Foo>, or even Box<StructWithDestructor> (I think?). This feels like an unprincipled hack that does not help any real-world cases.

@RalfJung
Copy link
Member Author

@arielb1

This PR only improves the situation with Box - it does not help with e.g. Vec, or even Box (I think?). This feels like an unprincipled hack that does not help any real-world cases.

Yeah... mostly I wrote this to confirm that this is indeed what's causing #46515. I was going to change it to fire any time the top-level type is Box, which at least could help some real-world cases, and then see if I can tweak your example to break it again.
But that's still way too limited. I am not sure what could be a good heuristic. Really #42771 is a hack that works around an LLVM deficiency, and trying to fix the bad side-effects of the hack without running into the LLVM misbehavior again is inadvertently going to be even more hacky.

@michaelwoerister
Copy link
Member

r? @arielb1

@arielb1, could I ask you to review this? I don't really know this code, so I don't know if anything subtle is going on here.

@arielb1
Copy link
Contributor

arielb1 commented Dec 14, 2017

@michaelwoerister

There is nothing subtle going on here - this just feels like a hack that is going to stay for quite some time without improving any real code.

@RalfJung
Copy link
Member Author

Do you think it is useful to try and develop a heuristic for this? Like, look at the type and allow inlining for the destructor if there are less than N nested drops?

Otherwise, I'm fine closing this.

@arielb1
Copy link
Contributor

arielb1 commented Dec 17, 2017

@RalfJung

I think developing an heuristic would be an interesting project but would require some work to actually help real-world cases. The test case in #36515 feels very artificial, so I don't feel an heuristic targeted at it would improve performance in the real world.

Therefore, I'm closing this PR until we can find something to base our heuristic on.

@arielb1 arielb1 closed this Dec 17, 2017
@RalfJung RalfJung deleted the drop branch July 10, 2018 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@arielb1 arielb1

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Missing optimization around __rust_alloc and unknown functions if panic=unwind
6 participants
@RalfJung @rust-highfive @kennytm @hanna-kruppe @arielb1 @michaelwoerister