Skip to content

/rust

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

Get LLVM using jemalloc and inline FNV hashing #31460

Merged
merged 3 commits into from Feb 14, 2016

Conversation

Reviewers
No reviews
Assignees

@brson brson

Labels
relnotes
Projects
None yet
Milestone
No milestone
6 participants
@alexcrichton @dotdash @bluss @brson @bors @nikomatsakis
@alexcrichton
Copy link
Member

alexcrichton commented Feb 7, 2016

Looking at some profiles of rustc recently, these seemed like two pretty easy wins we could get in terms of performance on the table.
@dotdash

This comment has been minimized.

Sign in to view
Copy link
Contributor

dotdash commented Feb 7, 2016

Do you have any numbers for this? Also, I seem to recall a huge discussion the last time the prefixes were to be dropped. Has that been resolved or is this a different change?
@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor

bluss commented Feb 7, 2016

We should put in a release note if the jemalloc prefix changes. If anyone is experimenting with JE_MALLOC_CONF and so on, it's useful to know it has changed.
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 7, 2016

@dotdash ah yeah sorry I forgot to copy them over into the PR description, but the two commits have:

jemalloc un-prefixing:

Locally this took compile time for libsyntax from 95 seconds to 89 (a 6% improvement).

fnv inline:

This commit made the hashing function disappear from a profiled version of the compiler, but that's likely because it was just inlined elsewhere. When compiling winapi, however, this decreased compile time from 18.3 to 17.8 seconds (a 3% improvement).

@bluss good point!

@alexcrichton alexcrichton added the relnotes label Feb 7, 2016

@dotdash

This comment has been minimized.

Sign in to view
Copy link
Contributor

dotdash commented Feb 7, 2016

@alexcrichton Thanks! You missed the prefix issue though. The old PR was #18678 which, besides the long discussion that led to it not being merged, also had it so that on iOS/OSX and Windows, the prefix is still present (which is the default for jemalloc).
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 7, 2016

Aha yes, thanks for finding a link to that! I should have hunted that down and thrown that in here.

When that discussion happened we did not have the same support for swapping out allocators that we have today. The RFC for custom allocators was merged long after that discussion, and the early conclusions also pointed towards using jemalloc unprefixed in executables. The major difference between now and #18678 is that we only use jemalloc in executables, no static library or dynamic library in Rust uses jemalloc by default (they use the system allocator).

As a result, due to the change in landscape, many of the concerns raised in #18678 have since been solved. One of the major motivational factors for RFC 1183 was indeed to land this change, I just forgot to get around to it until now!
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 8, 2016

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis Feb 8, 2016

@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Feb 9, 2016

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 9, 2016

📌 Commit 0e5c112 has been approved by brson
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 10, 2016

⌛️ Testing commit 0e5c112 with merge 9b24f54...

bors added a commit that referenced this pull request Feb 10, 2016

@bors
Auto merge of #31460 - alexcrichton:supafast-rustc, r=brson 
Looking at some profiles of rustc recently, these seemed like two pretty easy wins we could get in terms of performance on the table.
9b24f54
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 10, 2016

💔 Test failed - auto-mac-64-opt

@alexcrichton alexcrichton closed this Feb 10, 2016

@alexcrichton alexcrichton deleted the alexcrichton:supafast-rustc branch Feb 10, 2016

@alexcrichton alexcrichton restored the alexcrichton:supafast-rustc branch Feb 10, 2016

@alexcrichton alexcrichton reopened this Feb 10, 2016

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 10, 2016

Oops!
@dotdash

This comment has been minimized.

Sign in to view
Copy link
Contributor

dotdash commented Feb 10, 2016

@bors r-

Seems that bors was a bit too happy to see this restored

@alexcrichton alexcrichton force-pushed the alexcrichton:supafast-rustc branch from 0e5c112 to 51e0ee8 Feb 10, 2016

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 10, 2016

@bors: r=brson 51e0ee8

ok, let's try that again
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 11, 2016

⌛️ Testing commit 51e0ee8 with merge 59bff40...

bors added a commit that referenced this pull request Feb 11, 2016

@bors
Auto merge of #31460 - alexcrichton:supafast-rustc, r=brson 
Looking at some profiles of rustc recently, these seemed like two pretty easy wins we could get in terms of performance on the table.
59bff40
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 11, 2016

💔 Test failed - auto-linux-musl-64-opt
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 11, 2016

Looks like the failure on musl was legitimate, and it happened because:

  1. Both liballoc_jemalloc and liblibc defined a function called malloc
  2. When creating a binary with LTO, both rlibs were linked with -Wl,--whole-archive

This meant the linker found itself trying to include the same symbol twice, and rightly exited with an error.

I believe that the compiler here is erroneous in passing -Wl,--whole-archive when we're linking an executable (that was just intended for a dylib), so I've added a commit to not do that (with a test added as well).
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 11, 2016

ah and re-r? @brson

@rust-highfive rust-highfive assigned brson and unassigned nikomatsakis Feb 11, 2016

@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Feb 11, 2016

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 11, 2016

📌 Commit e93b127 has been approved by brson

@alexcrichton alexcrichton force-pushed the alexcrichton:supafast-rustc branch from 96429b6 to 00bf071 Feb 12, 2016

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 12, 2016

@bors: r=brson 00bf071

bors added a commit that referenced this pull request Feb 12, 2016

@bors
Auto merge of #31460 - alexcrichton:supafast-rustc, r=brson 
Looking at some profiles of rustc recently, these seemed like two pretty easy wins we could get in terms of performance on the table.
5978690
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 12, 2016

⌛️ Testing commit 00bf071 with merge 5978690...
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 12, 2016

@bors: retry force
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 12, 2016

⌛️ Testing commit 00bf071 with merge e733a31...

bors added a commit that referenced this pull request Feb 12, 2016

@bors
Auto merge of #31460 - alexcrichton:supafast-rustc, r=brson 
Looking at some profiles of rustc recently, these seemed like two pretty easy wins we could get in terms of performance on the table.
e733a31
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 12, 2016

💔 Test failed - auto-mac-64-nopt-t
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 12, 2016

@bors: retry

On Thu, Feb 11, 2016 at 9:53 PM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-mac-64-nopt-t
http://buildbot.rust-lang.org/builders/auto-mac-64-nopt-t/builds/8050

—
Reply to this email directly or view it on GitHub
#31460 (comment).
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 12, 2016

⌛️ Testing commit 00bf071 with merge 37ab094...

bors added a commit that referenced this pull request Feb 12, 2016

@bors
Auto merge of #31460 - alexcrichton:supafast-rustc, r=brson 
Looking at some profiles of rustc recently, these seemed like two pretty easy wins we could get in terms of performance on the table.
37ab094
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 12, 2016

💔 Test failed - auto-linux-64-x-android-t
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 12, 2016

@bors: retry

On Fri, Feb 12, 2016 at 4:51 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-64-x-android-t
http://buildbot.rust-lang.org/builders/auto-linux-64-x-android-t/builds/8006

—
Reply to this email directly or view it on GitHub
#31460 (comment).
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 13, 2016

⌛️ Testing commit 00bf071 with merge 27e3610...

bors added a commit that referenced this pull request Feb 13, 2016

@bors
Auto merge of #31460 - alexcrichton:supafast-rustc, r=brson 
Looking at some profiles of rustc recently, these seemed like two pretty easy wins we could get in terms of performance on the table.
27e3610
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 13, 2016

💔 Test failed - auto-linux-64-x-android-t
@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 14, 2016

Whelp, shoulda known that any segfault from unprefixing jemalloc is indicative of mixing allocators. Looks like on Android allocator replacement in libc somehow doesn't work. We call realpath with a NULL second parameter to indicate that it should return to us a buffer allocated with malloc. We then later call libc::free on the returned buffer. Presumably libc use the libc malloc, and we use the jemalloc free, hence badness!

I'll just re-prefix the symbols on android for now.

alexcrichton added some commits Feb 7, 2016

@alexcrichton
rustc: Ensure FNV hashing is inlined across crates 
Right now the primary hashing algorithm of the compiler isn't actually inlined
across crates, meaning that it may be missing out on some crucial optimizations
in a few places (perhaps unrolling smaller loops, etc).

This commit made the hashing function disappear from a profiled version of the
compiler, but that's likely because it was just inlined elsewhere. When
compiling winapi, however, this decreased compile time from 18.3 to 17.8 seconds
(a 3% improvement).
12a68e6
@alexcrichton
trans: Don't link whole rlibs to executables 
Back in 9bc8e6d the linking of rlibs changed to using the `link_whole_rlib`
function. This change, however was only intended to affect dylibs, not
executables. For executables we don't actually want to link entire rlibs because
we want the linker to strip out as much as possible.

This commit adds a conditional to this logic to only link entire rlibs if we're
creating a dylib, and otherwise an executable just links an rlib as usual. A
test is included which will fail to link if this behavior is reverted.
cc719d2
@alexcrichton
std: Stop prefixing jemalloc symbols 
Now that we properly only link in jemalloc when building executables, we have
far less to worry about in terms of polluting the global namespace with the
`free` and `malloc` symbols on Linux. This commit will primarily allow LLVM to
use jemalloc so the compiler will only be using one allocator overall.

Locally this took compile time for libsyntax from 95 seconds to 89 (a 6%
improvement).
Loading status checks…
e3b414d

@alexcrichton alexcrichton force-pushed the alexcrichton:supafast-rustc branch from 00bf071 to e3b414d Feb 14, 2016

@alexcrichton

This comment has been minimized.

Sign in to view
Copy link
Member Author

alexcrichton commented Feb 14, 2016

@bors: r=brson e3b414d
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 14, 2016

⌛️ Testing commit e3b414d with merge a888333...

bors added a commit that referenced this pull request Feb 14, 2016

@bors
Auto merge of #31460 - alexcrichton:supafast-rustc, r=brson 
Looking at some profiles of rustc recently, these seemed like two pretty easy wins we could get in terms of performance on the table.
a888333
@bors

@bors bors merged commit e3b414d into rust-lang:master Feb 14, 2016
2 checks passed

2 checks passed

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

@alexcrichton alexcrichton deleted the alexcrichton:supafast-rustc branch Feb 15, 2016

@bencord0 bencord0 referenced this pull request Aug 7, 2016

Closed

unrecognized relocation cargo build failures with latest nightlies #4

This was referenced Nov 13, 2017

Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.