Skip to content

Inline Box drop glue #46638

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

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).

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
Contributor

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

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

@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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing optimization around __rust_alloc and unknown functions if panic=unwind
6 participants