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

array.push(Foo { ... }) should construct Foo in-place #35531

Open
pcwalton opened this issue Aug 8, 2016 · 6 comments
Open

array.push(Foo { ... }) should construct Foo in-place #35531

pcwalton opened this issue Aug 8, 2016 · 6 comments
Labels
A-array Area: `[T; N]` A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pcwalton
Copy link
Contributor

pcwalton commented Aug 8, 2016

Right now when you push an immediate we construct a temporary copy on the stack and then move it into place on the heap. This is wasteful and results in a good deal of code bloat. Can we do better (with MIR perhaps)?

One possible sketch of a solution is to implement MIR inlining and then use some variant of #32966 that can prove that the construction of the argument to push() does not touch the vector, enabling us to forward the move from the stack to the destination of ptr::write().

An alternative would be to implement rust-lang/rfcs#1426.

cc @eddyb @nikomatsakis

@pcwalton pcwalton added I-slow Issue: Problems and improvements with respect to performance of generated code. A-optimization A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html labels Aug 8, 2016
@pcwalton
Copy link
Contributor Author

pcwalton commented Aug 8, 2016

See also #27779 and #30172

@eddyb
Copy link
Member

eddyb commented Aug 8, 2016

A simple test I put together quickly results in IR that looks like this:

  %6 = getelementptr inbounds %Struct, %Struct* %.pre, i64 %.pre.i, i32 0
  store i64 %1, i64* %6, align 8
  %7 = getelementptr inbounds %Struct, %Struct* %.pre, i64 %.pre.i, i32 1
  store i64 %2, i64* %7, align 8

There %.pre is the data pointer loaded from the Vec itself.

@pcwalton The problem occurs in more complex cases, could you try adjusting the one I linked to be more like what you're seeing copies with?

@nikomatsakis
Copy link
Contributor

I thought we had some initial impl of placement new?

@apasel422
Copy link
Contributor

CC #32366.

@arielb1
Copy link
Contributor

arielb1 commented Sep 25, 2016

Is there a code example?

@Mark-Simulacrum Mark-Simulacrum added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Jul 25, 2017
@ishitatsuyuki
Copy link
Contributor

Bump; move elimination is one thing we want. @nikomatsakis MIR inlining is available today, what additionally do we need?

@workingjubilee workingjubilee added the A-array Area: `[T; N]` label Mar 7, 2023
@Noratrieb Noratrieb added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-array Area: `[T; N]` A-MIR Area: Mid-level IR (MIR) - https://blog.rust-lang.org/2016/04/19/MIR.html C-enhancement Category: An issue proposing an enhancement or a PR with one. I-slow Issue: Problems and improvements with respect to performance of generated code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

9 participants