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

Poor inlining of closures for vec::from_fn<T>() #6623

Closed
dotdash opened this issue May 19, 2013 · 9 comments
Closed

Poor inlining of closures for vec::from_fn<T>() #6623

dotdash opened this issue May 19, 2013 · 9 comments

Comments

@dotdash
Copy link
Contributor

dotdash commented May 19, 2013

When there are multiple calls to vec::from_fn<T>() with the same type T but different closures, the compiler chooses to generate only one copy of vec::from_fn<T>() and pass the closure as an argument. This is really bad for simple closures like the ones in vec::from_elem<int>() or vec::to_owned<int>()

extern mod std;

#[bench]
fn to_owned(b: &mut std::test::BenchHarness) {
  let x = [0, ..1000];
  do b.iter {
    vec::to_owned(x);
  }
}

#[bench]
fn from_elem(b: &mut std::test::BenchHarness) {
  do b.iter {
    vec::from_elem(1000, 0);
  }
}

fn main() {
}

Running these gives:

running 2 tests
test from_elem ... bench: 2060 ns/iter (+/- 0)
test to_owned ... bench: 2306 ns/iter (+/- 3)

But if I remove one of the functions and only keep the other, I get:

running 1 test
test from_elem ... bench: 425 ns/iter (+/- 3)

and

running 1 test
test to_owned ... bench: 883 ns/iter (+/- 0)

This causes a 10% performance hit for rustc --no-trans. 0m14.664s vs. 13.592s with from_elem modified not to call from_fn but having a literal copy of the code.

@dotdash
Copy link
Contributor Author

dotdash commented Jun 2, 2013

Seems to be fixed now, probably by #6881

@dotdash dotdash closed this as completed Jun 2, 2013
@dotdash
Copy link
Contributor Author

dotdash commented Jun 2, 2013

For the record, I now get:

running 2 tests
test from_elem ... bench: 407 ns/iter (+/- 0)
test to_owned ... bench: 886 ns/iter (+/- 0)

result: ok. 0 passed; 0 failed; 0 ignored

@dotdash
Copy link
Contributor Author

dotdash commented Jun 2, 2013

Actually, @huonw made me aware that from_elem was adjusted to work around this issue. It's actually not fixed. New test code that only uses from_fn:

extern mod extra;

use std::vec;

#[bench]
fn test1(b: &mut extra::test::BenchHarness) {
    do b.iter {
        vec::from_fn(1000, |_| 0);
    }
}

#[bench]
fn test2(b: &mut extra::test::BenchHarness) {
    do b.iter {
        vec::from_fn(1000, |_| 1);
    }
}

fn main() {
}

Results:

running 2 tests
test test1 ... bench: 2064 ns/iter (+/- 6)
test test2 ... bench: 2071 ns/iter (+/- 0)

result: ok. 0 passed; 0 failed; 0 ignored


running 1 test
test test1 ... bench: 422 ns/iter (+/- 0)

result: ok. 0 passed; 0 failed; 0 ignored


running 1 test
test test2 ... bench: 423 ns/iter (+/- 0)

result: ok. 0 passed; 0 failed; 0 ignored

@emberian
Copy link
Member

emberian commented Aug 5, 2013

Still very relevant, reproduces.

@pcwalton
Copy link
Contributor

pcwalton commented Aug 9, 2013

Have you tried with -Z no-monomorphic-collapse?

@dotdash
Copy link
Contributor Author

dotdash commented Aug 9, 2013

I don't think I did, and I can't try right now, but I fail to see why that would possibly make a difference. The closures don't differ in their type signature but only in their constant return value. How would monomorphization make any difference?

@pcwalton
Copy link
Contributor

pcwalton commented Aug 9, 2013

Oh, I see, you're right.

@thestinger
Copy link
Contributor

I don't think this can be considered a bug, it's a consequence of only having boxed closures.

@thestinger
Copy link
Contributor

See #8622

flip1995 pushed a commit to flip1995/rust that referenced this issue Nov 4, 2021
new lint: string-slice

This is a restriction lint to highlight code that should have tests containing non-ascii characters. See rust-lang#6623.

changelog: new lint: [`string-slice`]
flip1995 pushed a commit to flip1995/rust that referenced this issue Nov 23, 2021
Add Clippy version to Clippy's lint list

Hey, hey, the semester is finally over, and I wanted to get back into hacking on Clippy. It has also been some time since our metadata collection monster has been feed. So, this PR adds a new attribute `clippy::version` to document which version a lint was stabilized. I considered using `git blame` but that would be very hacky and probably not accurate.

I'm also thinking that this attribute can be used to have a `clippy::nightly` lint group which is allow-by-default that delays setting the actual lint group until the defined version is reached. Just something to consider regarding rust-lang#6623 🙃

This PR only adds the version to 4 lints to keep it reviewable. I'll do a followup PR to add the version to other lints if the implementation is accepted 🙃

![image](https://user-images.githubusercontent.com/17087237/137118859-0aafdfdf-7595-4289-8ba4-33d58eb6991d.png)

Also, mobile approved xD

![image](https://user-images.githubusercontent.com/17087237/137118944-833cf7fb-a4a1-45d6-9af8-32c951822360.png)

---

r? `@flip1995`

cc: rust-lang#7172

closes: rust-lang#6492

changelog: [Clippy's lint list](https://rust-lang.github.io/rust-clippy/master/index.html) now displays the version a lint was added. 🎉

---

Example lint declaration after this update:

```rs
declare_clippy_lint! {
    /// [...]
    ///
    /// ### Example
    /// ```rust
    /// // Bad
    /// let x = 3.14;
    /// // Good
    /// let x = std::f32::consts::PI;
    /// ```
    #[clippy::version = "pre 1.29.0"]
    pub APPROX_CONSTANT,
    correctness,
    "the approximate of a known float constant (in `std::fXX::consts`)"
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants