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

Revert "Use an UTF-8 locale for the linker." #74478

Merged
merged 1 commit into from
Jul 19, 2020

Conversation

jonas-schievink
Copy link
Contributor

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2020
@jonas-schievink
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jul 18, 2020

⌛ Trying commit 09240a4 with merge 556b0eb41b4273f0dcde2cbc2bc9f53e2c09d688...

@bors
Copy link
Contributor

bors commented Jul 18, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 556b0eb41b4273f0dcde2cbc2bc9f53e2c09d688 (556b0eb41b4273f0dcde2cbc2bc9f53e2c09d688)

@rust-timer
Copy link
Collaborator

Queued 556b0eb41b4273f0dcde2cbc2bc9f53e2c09d688 with parent d3df851, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (556b0eb41b4273f0dcde2cbc2bc9f53e2c09d688): comparison url.

@jonas-schievink
Copy link
Contributor Author

Yeah that was it.

r? @oli-obk

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 18, 2020

📌 Commit 09240a4 has been approved by Mark-Simulacrum

@bors 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 Jul 18, 2020
@Manishearth
Copy link
Member

@bors p=1

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2020
…arth

Rollup of 7 pull requests

Successful merges:

 - rust-lang#70817 (Add core::task::ready! macro)
 - rust-lang#73762 (Document the trait keyword)
 - rust-lang#74021 (impl Index<RangeFrom> for CStr)
 - rust-lang#74071 (rustc_metadata: Make crate loading fully speculative)
 - rust-lang#74445 (add test for rust-lang#62878)
 - rust-lang#74459 (Make unreachable_unchecked a const fn)
 - rust-lang#74478 (Revert "Use an UTF-8 locale for the linker.")

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Jul 19, 2020

⌛ Testing commit 09240a4 with merge 0701419...

@bors bors merged commit a83e294 into master Jul 19, 2020
@jonas-schievink jonas-schievink deleted the revert-74416-linker-locale-utf8 branch July 19, 2020 11:42
@nnethercote
Copy link
Contributor

This 100% should have not landed in a rollup. It had an obvious and very large performance improvement. It's quite possible that this improvement masked a regression caused by another PR in the rollup.

Rollup application has become a lot more aggressive recently, and this week it is causing all sorts of problems in diagnosing performance regressions.

@Manishearth: please avoid including PRs that obviously have perf effects in rollups. Looking for perf CI runs ("comparison URL") is one thing to check for. (Watch out for GitHub hiding comments when the PR gets long.)

@jonas-schievink: please use "rollup=never" to mark PRs that have known perf effects.

@Manishearth
Copy link
Member

@nnethercote yeah, I've been more aggressive because the queue has gotten bigger a couple times.

I try to avoid including perf-affecting PRs in rollups, it's just easier if they're marked as rollup=never. In this case I figured we already knew what the impact was so wouldn't care. I guess I misunderstood the reasons behind avoiding rollups here (namely, masking the perf impact of the other PRs)

I don't think it's reasonable for me to look through comments for this, if there's a perf impact that you care to measure separately, please use rollup=never.

@nnethercote
Copy link
Contributor

I don't think it's reasonable for me to look through comments for this

Even when there is a fixed string you can look for?

My perf triage this week is an utter disaster. So many regressions, almost all in rollups. Some rollups with multiple regressions. Some rollups with both improvements and regressions. I don't even know for sure how many regressions we've had. I fear that we might not be able to determine them all without heroics, and we simply won't get perf back to where it was a week ago due to lack of information.

I understand that rollups are unavoidable, but in my opinion the current approach is untenable when it comes to avoiding performance regressions, and I will be starting discussions about what we can do to improve things moving forward. In the meantime, if you are able to reduce the number of PRs per rollup -- even 10 is better than 18, for example -- that would also be helpful.

@Manishearth
Copy link
Member

Even when there is a fixed string you can look for?

In multiple PRs, many of which have the "show hidden comments" thing. Perhaps there should be tooling to automatically rollup=never any PR when it gets a perf run? (which can be undone if necessary) Like, that flag exists precisely as a signal so that rollup authors don't need to do this.

Anyway, part of the issue was that I did not know that "may significantly impact perf" was a reason to always exclude from rollups in the first place. I knew that some perf PRs were rollup=never'd, but I didn't realize the reason behind that was that otherwise it's hard to triage the perf impact of the rollup. I thought the reason was simply that the authors desired a clean perf page for the PR.

So I can definitely keep an eye out and proactively rollup=never things that affect perf. I already proactively do that kind of thing for funky build system PRs.

if you are able to reduce the number of PRs per rollup -- even 10 is better than 18, for example -- that would also be helpful.

I can try, the problem is that a single PR takes four hours to test, and there's significant shepherding, so it ends up being like one rollup a day, letting rollup=nevers (which also pile up) land overnight.

Note that most rollups have a relatively small number of nontrivial PRs, the rollups of size 18 typically contain the same number of nontrivial PRs as the rollups of size 10 -- when making a rollup I'll select a small number of iffy/maybe PRs and then basically everything in the rollup=always section. I can change this part very easily, the rollup=always PRs typically don't matter much.

@nnethercote
Copy link
Contributor

In multiple PRs, many of which have the "show hidden comments" thing. Perhaps there should be tooling to automatically rollup=never any PR when it gets a perf run? (which can be undone if necessary)

Yes, this is a good idea.

Anyway, part of the issue was that I did not know that "may significantly impact perf" was a reason to always exclude from rollups in the first place.

Is there documentation on how to do rollups, and manage landings in general? It would be good to have, to make it easier for new people who take on the role. E.g. for perf triage I have these docs, which I have updated several times.

So I can definitely keep an eye out and proactively rollup=never things that affect perf. I already proactively do that kind of thing for funky build system PRs.

Great, thank you.

Note that most rollups have a relatively small number of nontrivial PRs, the rollups of size 18 typically contain the same number of nontrivial PRs as the rollups of size 10 -- when making a rollup I'll select a small number of iffy/maybe PRs and then basically everything in the rollup=always section. I can change this part very easily, the rollup=always PRs typically don't matter much.

Ok. My sense is that the "is trivial" judgment is harder than it seems, for anybody. So a rollup with 18 is much more likely to have one or more seems-trivial-but-isn't PRs in it than a rollup with 9.

@Manishearth
Copy link
Member

Is there documentation on how to do rollups, and manage landings in general?

I've been meaning to write some.

My sense is that the "is trivial" judgment is harder than it seems, for anybody

I typically just trust the categorization, but I can also look closer at them.

@nnethercote
Copy link
Contributor

I typically just trust the categorization, but I can also look closer at them.

What's that? Is it a label the author puts on it?

@Manishearth
Copy link
Member

@nnethercote no, I meant the categorization of rollup=always

but I used to do rollups before we had this categorization, so I have a decent sense of what might be less than trivial for perf or for other reasons

@Mark-Simulacrum
Copy link
Member

@rust-timer make-pr-for a83e294

This should hopefully confirm my latest theory that this is indeed entirely responsible for that rollups results.

rust-timer added a commit to rust-timer/rust that referenced this pull request Jul 23, 2020
Original message:
Rollup merge of rust-lang#74478 - rust-lang:revert-74416-linker-locale-utf8, r=Mark-Simulacrum

Revert "Use an UTF-8 locale for the linker."

Reverts rust-lang#74416

This is suspected to have caused significant compile time regressions: https://perf.rust-lang.org/compare.html?start=39d5a61f2e4e237123837f5162cc275c2fd7e625&end=d3df8512d2c2afc6d2e7d8b5b951dd7f2ad77b02&stat=instructions:u
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants