[WIP] Make `into` schedule drop for the destination again #65385

matthewjasper · 2019-10-13T17:06:32Z

#61430 triggered some degenerate behavior in llvm where it would inline functions far too aggressively.

This is marked as WIP because it doesn't really solve the problem in general. I've opened this PR more so that there's a place to discuss fixes.

Minimized example of the problem

Uncommenting the #[inline] will cause this to take a long time to compile on nightly, since llvm will inline all of the next calls into one enormous function that it spends a very long time handling.

pub trait Iterator {
    type Item;
    
    fn next(&mut self) -> Self::Item;
}

pub struct Empty;

impl Iterator for Empty {
    type Item = ();
    fn next(&mut self) {}
}

pub struct Chain<A, B=Empty> {
    a: A,
    b: B,
    state: ChainState,
}

#[allow(dead_code)]
enum ChainState {
    Both,
    Front,
    Back,
}

impl<A, B> Iterator for Chain<A, B> where
    A: Iterator,
    B: Iterator<Item = A::Item>
{
    type Item = A::Item;

    //#[inline]
    //^ uncomment me for degenerate llvm behvaiour with `-O`
    fn next(&mut self) -> A::Item {
        let ret;
        match self.state {
            ChainState::Both => {
                let _x = self.a.next();
                self.state = ChainState::Back;
                ret = self.b.next()
            },
            ChainState::Front => ret = self.a.next(),
            ChainState::Back => ret = self.b.next(),
        };
        ret
    }
}

type Chain2<T> = Chain<Chain<T>>;
type Chain4<T> = Chain2<Chain2<T>>;
type Chain8<T> = Chain4<Chain4<T>>;
type Chain16<T> = Chain8<Chain8<T>>;

pub fn call_next(x: &mut Chain16<Empty>) {
    x.next()
}

Closes #47949

rust-highfive · 2019-10-13T17:06:36Z

r? @eddyb

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

matthewjasper · 2019-10-13T21:22:11Z

@bors try @rust-timer queue

rust-timer · 2019-10-13T21:22:12Z

Awaiting bors try build completion

bors · 2019-10-13T21:22:21Z

⌛ Trying commit ac5531a84c6549f74a7d1ba772cf2ffafca4a03f with merge 657db25f5495ead2ced9bac02891196b47dac8a3...

Centril · 2019-10-13T21:35:35Z

@bors retry try

bors · 2019-10-13T21:35:59Z

⌛ Trying commit ac5531a84c6549f74a7d1ba772cf2ffafca4a03f with merge 9a46f242f60618bf42c2d0ae1d088307b9464ba1...

bors · 2019-10-14T00:40:13Z

☀️ Try build successful - checks-azure
Build commit: 9a46f242f60618bf42c2d0ae1d088307b9464ba1 (9a46f242f60618bf42c2d0ae1d088307b9464ba1)

rust-timer · 2019-10-14T00:40:14Z

Queued 9a46f242f60618bf42c2d0ae1d088307b9464ba1 with parent c27f756, future comparison URL.

gnzlbg · 2019-10-14T10:16:30Z

src/libcore/iter/adapters/chain.rs

@@ -54,7 +54,6 @@ impl<A, B> Iterator for Chain<A, B> where
 {
    type Item = A::Item;

-    #[inline]


With lto (thin or fat), these attributes shouldn't really make much of a difference. Is the performance regression still there when these are present ?

These attributes were the cause of the perf regression

How is that possible? The PR that introduced the regression did not add these attributes, so they must have already been there, right ? Without that PR, these attributes were not introducing that regression, so something else in that PR caused it.

rust-timer · 2019-10-14T15:23:27Z

Finished benchmarking try commit 9a46f242f60618bf42c2d0ae1d088307b9464ba1, comparison URL.

mati865 · 2019-10-14T19:08:57Z

Still regresses badly, especially deeply-nested-opt.

JohnCSimon · 2019-10-26T01:54:10Z

Ping from triage -
@mati865 Can you please post the status of this PR? Also you have some merge conflicts.
cc: @eddyb @gnzlbg

Thanks!

mati865 · 2019-10-26T10:52:43Z

You probably mean to ask @matthewjasper

JohnCSimon · 2019-11-02T01:13:12Z

Ping from triage -
@matthewjasper Can you please post the status of this PR? Also you have some merge conflicts.
cc: @eddyb @gnzlbg

Thanks!

This reverts commit a0342c8

rust-highfive · 2019-11-02T10:54:24Z

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-11-02T10:36:23.2144980Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-02T10:36:23.2344152Z ##[command]git config gc.auto 0
2019-11-02T10:36:23.2417877Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-02T10:36:23.2473935Z ##[command]git config --get-all http.proxy
2019-11-02T10:36:23.2622235Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65385/merge:refs/remotes/pull/65385/merge
---
2019-11-02T10:42:03.1811906Z    Compiling serde_json v1.0.40
2019-11-02T10:42:04.6769286Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-11-02T10:42:14.4485428Z     Finished release [optimized] target(s) in 1m 15s
2019-11-02T10:42:14.4558727Z tidy check
2019-11-02T10:42:14.8431677Z tidy error: /checkout/src/librustc_mir/dataflow/move_paths/builder.rs:359: TODO is deprecated; use FIXME
2019-11-02T10:42:16.4383434Z some tidy checks failed
2019-11-02T10:42:16.4387518Z Found 485 error codes
2019-11-02T10:42:16.4387619Z Found 0 error codes with no tests
2019-11-02T10:42:16.4388749Z Done!
2019-11-02T10:42:16.4388749Z Done!
2019-11-02T10:42:16.4388807Z 
2019-11-02T10:42:16.4388834Z 
2019-11-02T10:42:16.4389736Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-11-02T10:42:16.4389866Z 
2019-11-02T10:42:16.4389911Z 
2019-11-02T10:42:16.4399907Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-11-02T10:42:16.4400001Z Build completed unsuccessfully in 0:01:18
2019-11-02T10:42:16.4400001Z Build completed unsuccessfully in 0:01:18
2019-11-02T10:42:16.4450592Z == clock drift check ==
2019-11-02T10:42:16.4457950Z   local time: Sat Nov  2 10:42:16 UTC 2019
2019-11-02T10:42:16.5813137Z   network time: Sat, 02 Nov 2019 10:42:16 GMT
2019-11-02T10:42:16.5818945Z == end clock drift check ==
2019-11-02T10:42:17.9651889Z 
2019-11-02T10:42:17.9770797Z ##[error]Bash exited with code '1'.
2019-11-02T10:42:17.9795221Z ##[section]Starting: Checkout
2019-11-02T10:42:17.9796977Z ==============================================================================
2019-11-02T10:42:17.9797056Z Task         : Get sources
2019-11-02T10:42:17.9797099Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

matthewjasper · 2019-11-02T14:15:19Z

Trying one more time before I close this and work on a different approach
@bors try @rust-timer queue

rust-timer · 2019-11-02T14:15:21Z

Awaiting bors try build completion

bors · 2019-11-02T14:15:33Z

⌛ Trying commit 7d7dc00 with merge 8712d90...

[WIP] Make `into` schedule drop for the destination again #61430 triggered some degenerate behavior in llvm where it would inline functions far too aggressively. This is marked as WIP because it doesn't really solve the problem in general. I've opened this PR more so that there's a place to discuss fixes. <details> <summary>Minimized example of the problem</summary> Uncommenting the `#[inline]` will cause this to take a long time to compile on nightly, since llvm will inline all of the next calls into one enormous function that it spends a very long time handling. ```rust pub trait Iterator { type Item; fn next(&mut self) -> Self::Item; } pub struct Empty; impl Iterator for Empty { type Item = (); fn next(&mut self) {} } pub struct Chain<A, B=Empty> { a: A, b: B, state: ChainState, } #[allow(dead_code)] enum ChainState { Both, Front, Back, } impl<A, B> Iterator for Chain<A, B> where A: Iterator, B: Iterator<Item = A::Item> { type Item = A::Item; //#[inline] //^ uncomment me for degenerate llvm behvaiour with `-O` fn next(&mut self) -> A::Item { let ret; match self.state { ChainState::Both => { let _x = self.a.next(); self.state = ChainState::Back; ret = self.b.next() }, ChainState::Front => ret = self.a.next(), ChainState::Back => ret = self.b.next(), }; ret } } type Chain2<T> = Chain<Chain<T>>; type Chain4<T> = Chain2<Chain2<T>>; type Chain8<T> = Chain4<Chain4<T>>; type Chain16<T> = Chain8<Chain8<T>>; pub fn call_next(x: &mut Chain16<Empty>) { x.next() } ``` </details> Closes #47949

bors · 2019-11-02T17:00:50Z

☀️ Try build successful - checks-azure
Build commit: 8712d90 (8712d9047a7c1d95447f71ae89b1596bb953d798)

rust-timer · 2019-11-02T17:00:52Z

Queued 8712d90 with parent f39205b, future comparison URL.

rust-timer · 2019-11-03T02:10:33Z

Finished benchmarking try commit 8712d90, comparison URL.

rust-highfive assigned eddyb Oct 13, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2019

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 Oct 13, 2019

gnzlbg reviewed Oct 14, 2019

View reviewed changes

matthewjasper added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 19, 2019

Make into schedule drop for the destination (again)

37e0352

This reverts commit a0342c8

matthewjasper force-pushed the drop-on-into-panic-otp branch from ac5531a to 98c3371 Compare November 2, 2019 10:34

matthewjasper added 2 commits November 2, 2019 12:16

Reduce the number of drop-flag assignments in unwind paths

4229e2b

Remove some unnecessary inline attributes

7d7dc00

matthewjasper force-pushed the drop-on-into-panic-otp branch from 98c3371 to 7d7dc00 Compare November 2, 2019 12:16

matthewjasper closed this Nov 3, 2019

matthewjasper deleted the drop-on-into-panic-otp branch November 3, 2019 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Make `into` schedule drop for the destination again #65385

[WIP] Make `into` schedule drop for the destination again #65385

matthewjasper commented Oct 13, 2019

rust-highfive commented Oct 13, 2019

matthewjasper commented Oct 13, 2019

rust-timer commented Oct 13, 2019

bors commented Oct 13, 2019

Centril commented Oct 13, 2019

bors commented Oct 13, 2019

bors commented Oct 14, 2019

rust-timer commented Oct 14, 2019

gnzlbg Oct 14, 2019

matthewjasper Oct 14, 2019

gnzlbg Oct 14, 2019 •

edited

Loading

rust-timer commented Oct 14, 2019

mati865 commented Oct 14, 2019

JohnCSimon commented Oct 26, 2019

mati865 commented Oct 26, 2019

JohnCSimon commented Nov 2, 2019

rust-highfive commented Nov 2, 2019

matthewjasper commented Nov 2, 2019

rust-timer commented Nov 2, 2019

bors commented Nov 2, 2019

bors commented Nov 2, 2019

rust-timer commented Nov 2, 2019

rust-timer commented Nov 3, 2019

[WIP] Make into schedule drop for the destination again #65385

[WIP] Make into schedule drop for the destination again #65385

Conversation

matthewjasper commented Oct 13, 2019

rust-highfive commented Oct 13, 2019

matthewjasper commented Oct 13, 2019

rust-timer commented Oct 13, 2019

bors commented Oct 13, 2019

Centril commented Oct 13, 2019

bors commented Oct 13, 2019

bors commented Oct 14, 2019

rust-timer commented Oct 14, 2019

gnzlbg Oct 14, 2019

Choose a reason for hiding this comment

matthewjasper Oct 14, 2019

Choose a reason for hiding this comment

gnzlbg Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

rust-timer commented Oct 14, 2019

mati865 commented Oct 14, 2019

JohnCSimon commented Oct 26, 2019

mati865 commented Oct 26, 2019

JohnCSimon commented Nov 2, 2019

rust-highfive commented Nov 2, 2019

matthewjasper commented Nov 2, 2019

rust-timer commented Nov 2, 2019

bors commented Nov 2, 2019

bors commented Nov 2, 2019

rust-timer commented Nov 2, 2019

rust-timer commented Nov 3, 2019

[WIP] Make `into` schedule drop for the destination again #65385

[WIP] Make `into` schedule drop for the destination again #65385

gnzlbg Oct 14, 2019 •

edited

Loading