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

Use MaybeUninit in libcore #54668

Merged
merged 7 commits into from Nov 27, 2018

Conversation

Projects
None yet
@RalfJung
Member

RalfJung commented Sep 29, 2018

All code by @japaric. This re-submits the second half of #53508 (the first half is at #54667). This is likely the one containing the perf regression.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Sep 29, 2018

r? @dtolnay

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

@RalfJung

This comment has been minimized.

Member

RalfJung commented Sep 29, 2018

@bors try

@bors

This comment has been minimized.

Contributor

bors commented Sep 29, 2018

⌛️ Trying commit 5f7792b with merge 5e4ca24...

bors added a commit that referenced this pull request Sep 29, 2018

Auto merge of #54668 - RalfJung:use-maybe-uninit, r=<try>
Use MaybeUninit

This re-submits the second half of #53508 (the first half is at #54667). This is likely the one containing the perf regression.

@RalfJung RalfJung force-pushed the RalfJung:use-maybe-uninit branch 2 times, most recently from b80c934 to e51b814 Sep 29, 2018

@RalfJung

This comment has been minimized.

Member

RalfJung commented Sep 29, 2018

(Looks like try is done but there was no notification about that? @rust-lang/infra )

@rust-timer build 5e4ca24

@RalfJung

This comment was marked as outdated.

Member

RalfJung commented Sep 29, 2018

@rust-timer build 5e4ca24

(Command must be in first line)

@rust-timer

This comment was marked as resolved.

rust-timer commented Sep 29, 2018

Please provide the full 40 character commit hash.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Sep 29, 2018

@rust-timer build 5e4ca24

(And there is no good way to copy-paste the commit SHA in one step)

@rust-timer

This comment has been minimized.

rust-timer commented Sep 29, 2018

Success: Queued 5e4ca24 with parent 7e7bc06, comparison URL.

@dtolnay

This comment has been minimized.

Member

dtolnay commented Sep 29, 2018

@rust-highfive rust-highfive assigned RalfJung and unassigned dtolnay Sep 29, 2018

@RalfJung

This comment has been minimized.

Member

RalfJung commented Sep 29, 2018

@dtolnay Eh, I submitted this. I do not think I should review it.^^

@dtolnay

This comment has been minimized.

Member

dtolnay commented Sep 29, 2018

You already reviewed this code in #53508. Please clarify what remains to be reviewed.

Assigning the same reviewer as #54667:
r? @cramertj

@rust-highfive rust-highfive assigned cramertj and unassigned RalfJung Sep 29, 2018

@RalfJung

This comment has been minimized.

Member

RalfJung commented Sep 29, 2018

I wasn't aware of a policy for re-submissions of reverted PRs that would let me r+ this even though I assembled the commits.

And anyway I expect this PR will need changes because there will be perf regressions. But let's see.

@Valinora

This comment has been minimized.

Valinora commented Sep 30, 2018

Perf results are in, this definitely looks to be the source of the regression.

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Oct 2, 2018

@RalfJung I don't know if this helps, but I don't see anything suspicious with this PR, so maybe the issue causing the regression is not here?

One "suspicious" thing is that the regression happens in liballoc, but MaybeUninit is declared in libcore and its methods are not #[inline], which means that they cannot reliably be inlined into other crates without some form of LTO. The functions being touched here are big, so maybe LTO is not inlining these?

If you look at the disassembly of these tests and see a call instruction pointing to the MaybeUninit methods in libcore, it might be worth testing whether making MaybeUninit methods #[inline] might fix this regression.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Oct 2, 2018

It's methods are generic though so #[inline] shouldn't make a difference, right?

the regression happens in liballoc

Where are you getting that from?

Unfortunately I will not have much time to work on this this week.

@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Oct 2, 2018

It's methods are generic though so #[inline] shouldn't make a difference, right?

Indeed. The only way in which that could make a difference is if they call a non-generic function that is not #[inline], but it appears that this is not the case. I'm out of ideas.

Where are you getting that from?

I thought most of the changes here were in liballoc, but there are some changes in libcore as well. It might be worth it to figure out which of the changes (slice rotate, slice sort, ptr, liballoc's btree, etc.) introduces the regression.

@RalfJung RalfJung force-pushed the RalfJung:use-maybe-uninit branch from e51b814 to a573bfc Oct 8, 2018

@RalfJung

This comment has been minimized.

Member

RalfJung commented Oct 8, 2018

I might be doing the wrong thing, but I failed to reproduce the perf regression... in fact, issue-46449 got 12% faster with this PR in my measurement:

$ hyperfine -w 1 -m 3 -p 'rm target -rf' 'cargo +stage1 build --release' 'cargo +stage1.2 build --release'
Benchmark #1: cargo +stage1 build --release

  Time (mean ± σ):      3.532 s ±  0.014 s    [User: 6.576 s, System: 0.115 s]
 
  Range (min … max):    3.520 s …  3.548 s
 
Benchmark #2: cargo +stage1.2 build --release

  Time (mean ± σ):      3.166 s ±  0.059 s    [User: 6.180 s, System: 0.137 s]
 
  Range (min … max):    3.122 s …  3.233 s
 
Summary

  'cargo +stage1.2 build --release' ran
    1.12x faster than 'cargo +stage1 build --release'
@rust-timer

This comment has been minimized.

rust-timer commented Nov 21, 2018

Success: Queued 4fe0b8e with parent 289ad6e, comparison URL.

@ogoffart

This comment has been minimized.

Contributor

ogoffart commented Nov 21, 2018

Keep in mind that if this is happening in cargo check testcases, all you'll be seeing is how fast or slow rustc is, with LLVM not being involed at all.

If I read the number correctly, the "-check" benchmark does not seem to be impacted. The red numbers are only in the "-opt" and "-debug" benchmarks

@rust-timer

This comment has been minimized.

rust-timer commented Nov 21, 2018

Finished benchmarking try commit 4fe0b8e

kennytm added a commit to kennytm/rust that referenced this pull request Nov 23, 2018

Rollup merge of rust-lang#56097 - ogoffart:union-abi, r=eddyb
Fix invalid bitcast taking bool out of a union represented as a scalar

As reported in rust-lang#54668 (comment)

RalfJung added some commits Oct 10, 2018

use MaybeUninit in core::fmt
Code by @japaric, I just split it into individual commits
use MaybeUninit in core::slice::sort
Code by @japaric, I just split it into individual commits
use MaybeUninit in core::slice::rotate
Code by @japaric, I just split it into individual commits
use MaybeUninit in core::ptr::swap_nonoverlapping_bytes
Code by @japaric, I just split it into individual commits
use MaybeUninit in core::ptr::{read,read_unaligned}
Code by @japaric, I just split it into individual commits
use MaybeUninit in core::ptr::swap
Code by @japaric, I just split it into individual commits

@RalfJung RalfJung force-pushed the RalfJung:use-maybe-uninit branch from c400e50 to 59786b0 Nov 23, 2018

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 23, 2018

Unions got fixed, I rebased this PR.

So, can we land this?

@ogoffart

(I'm in no position to review, but i see no problem with this, and since it is required to stabilize MaybeUnitit, it'd be great to have it in as early as possible)

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Nov 26, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 26, 2018

📌 Commit 59786b0 has been approved by SimonSapin

@bors

This comment has been minimized.

Contributor

bors commented Nov 26, 2018

⌛️ Testing commit 59786b0 with merge 75d937c...

bors added a commit that referenced this pull request Nov 26, 2018

Auto merge of #54668 - RalfJung:use-maybe-uninit, r=SimonSapin
Use MaybeUninit in libcore

All code by @japaric. This re-submits the second half of #53508 (the first half is at #54667). This is likely the one containing the perf regression.
@bors

This comment has been minimized.

Contributor

bors commented Nov 27, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: SimonSapin
Pushing 75d937c to master...

@bors bors merged commit 59786b0 into rust-lang:master Nov 27, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@gnzlbg

This comment has been minimized.

Contributor

gnzlbg commented Nov 27, 2018

Finally, awesome, thanks!

@RalfJung RalfJung deleted the RalfJung:use-maybe-uninit branch Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment