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

Downgrade bundled jemalloc version #30985

Merged
merged 1 commit into from
Jan 20, 2016

Conversation

alexcrichton
Copy link
Member

We've been seeing a lot of timeouts in tests on the bots and investigation ended
pointing to jemalloc/jemalloc#315 as the culprit. Unfortunately it looks like
that doesn't seem to have a fix on the way soon, so let's temporarily downgrade
back to the previous version of jemalloc we were using (where #30434 was the
most recent upgrade)

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@alexcrichton
Copy link
Member Author

cc @aturon, @brson

@Gankra
Copy link
Contributor

Gankra commented Jan 17, 2016

@bors r+

Might as well run the bugs that we didn't run into.

@bors
Copy link
Contributor

bors commented Jan 17, 2016

📌 Commit ab758e5 has been approved by Gankro

@Gankra
Copy link
Contributor

Gankra commented Jan 17, 2016

@bors p=1

@bors
Copy link
Contributor

bors commented Jan 17, 2016

⌛ Testing commit ab758e5 with merge 6834008...

@bors
Copy link
Contributor

bors commented Jan 17, 2016

💔 Test failed - auto-win-gnu-64-nopt-t

@alexcrichton
Copy link
Member Author

Hm, this is... interesting. We seem to be in a bit of a quandry with respect to jemalloc on Windows. I suspect that this is jemalloc/jemalloc#310 coming to haunt us again.

In the past the --enable-lazy-lock option was passed to jemalloc to fix some bug which I believe is being exposed here, but I also believe that the "lazy locking" semantics are papering over the real underlying bug. I suspect this "real bug" is related to how the locking code in jemalloc is implemented on Windows (which has changed quite a bit from this older rev to the current head of jemalloc), but I have not confirmed that one way or another.

All in all, it appears that --disable-lazy-lock segfaults with older jemalloc versions (as is evidenced by these build logs), and --enable-lazy-lock is fundamentally flawed on Windows (as I mentioned in the linked issue). For that reason, in this downgrading of jemalloc I think that we may want to disable it for the GNU Windows targets.

Thoughts @rust-lang/tools? Opinions on disabling jemalloc for Windows temporarily until we can re-upgrade again?

@vadimcn
Copy link
Contributor

vadimcn commented Jan 19, 2016

Well, if it breaks stuff, we should disable it. Are we gonna lose on some important benchmark because of this?

@alexcrichton
Copy link
Member Author

I suspect we'll looks on some benchmark as jemalloc is probably faster than the system allocator for at least some workloads, but I also agree that it's likely more important to work at all!

@nikomatsakis
Copy link
Contributor

Seems to me we should disable jemalloc on windows, yes. It'd be great if we can make a corresponding Rust issue with the details and citations you've dumped on this thread, so that we have a central place to track this. (Would this mean that we would enable the newer jemalloc elsewhere? I'm guess not, because jemalloc/jemalloc#315 doesn't appear to be specific to Windows.)

@alexcrichton
Copy link
Member Author

Ok, I've filed an issue and disabled jemalloc on windows-gnu.

r? @brson

@rust-highfive rust-highfive assigned brson and unassigned nikomatsakis Jan 19, 2016
@brson
Copy link
Contributor

brson commented Jan 19, 2016

@bors r+ p=1

@bors
Copy link
Contributor

bors commented Jan 19, 2016

📌 Commit 840a1d7 has been approved by brson

@bors
Copy link
Contributor

bors commented Jan 20, 2016

⌛ Testing commit 840a1d7 with merge 05756dd...

@bors
Copy link
Contributor

bors commented Jan 20, 2016

💔 Test failed - auto-win-gnu-32-nopt-t

@alexcrichton
Copy link
Member Author

@bors: r=brson 66cdd85

@bors
Copy link
Contributor

bors commented Jan 20, 2016

⌛ Testing commit 66cdd85 with merge 5a50f7f...

@bors
Copy link
Contributor

bors commented Jan 20, 2016

💔 Test failed - auto-win-gnu-64-opt

We've been seeing a lot of timeouts in tests on the bots and investigation ended
pointing to jemalloc/jemalloc#315 as the culprit. Unfortunately it looks like
that doesn't seem to have a fix on the way soon, so let's temporarily downgrade
back to the previous version of jemalloc we were using (where rust-lang#30434 was the
most recent upgrade)
@alexcrichton
Copy link
Member Author

@bors: r=brson 884de56

bors added a commit that referenced this pull request Jan 20, 2016
We've been seeing a lot of timeouts in tests on the bots and investigation ended
pointing to jemalloc/jemalloc#315 as the culprit. Unfortunately it looks like
that doesn't seem to have a fix on the way soon, so let's temporarily downgrade
back to the previous version of jemalloc we were using (where #30434 was the
most recent upgrade)
@bors
Copy link
Contributor

bors commented Jan 20, 2016

⌛ Testing commit 884de56 with merge 0b77e50...

@bors bors merged commit 884de56 into rust-lang:master Jan 20, 2016
@alexcrichton alexcrichton deleted the downgrade-jemaloc branch January 21, 2016 01:30
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

Successfully merging this pull request may close these issues.

7 participants