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

Try to fix a perf regression by updating log #47161

Merged
merged 1 commit into from
Jan 7, 2018

Conversation

MaloJaffre
Copy link
Contributor

@MaloJaffre MaloJaffre commented Jan 3, 2018

Upgrade log to 0.4 in multiple crates and cargo.
Cc: #47154.

@rust-highfive
Copy link
Collaborator

r? @arielb1

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

@MaloJaffre MaloJaffre changed the title [WIP] Investigate a regression by updating log and env_logger Try to fix a perf regression by updating log Jan 3, 2018
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jan 4, 2018

📌 Commit 773c60a has been approved by alexcrichton

@alexcrichton alexcrichton added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 4, 2018
@kennytm
Copy link
Member

kennytm commented Jan 6, 2018

@bors p=4

(Try to see if this can reduce the recent timeout failures)

@bors
Copy link
Contributor

bors commented Jan 6, 2018

⌛ Testing commit 773c60a98e971c4ceba6e7c1bfc0d0286eb1384a with merge fbc632342d9c510f4b01d79e7364f220b89165f2...

@bors
Copy link
Contributor

bors commented Jan 7, 2018

💔 Test failed - status-appveyor

@kennytm
Copy link
Member

kennytm commented Jan 7, 2018

A legit error caused by bad interaction between the C library libgit2 and the updated winapi on x86_64-pc-windows-gnu.

The gai_strerrorA function is undefined, probably because winapi 0.3 no longer automatically links to all Windows libraries (you need to opt-in for the features). In particular the feature ws2tcpip which imports winsock2.lib is optional. However libgit2-sys itself does not depend on winapi, so previously it was just "accidentally working"? cc @alexcrichton

   Compiling cargo v0.25.0 (file:///C:/projects/rust/src/tools/cargo)
error: linking with `gcc` failed: exit code: 1
  |
  = note: <snip>
  = note: C:\projects\rust\build\x86_64-pc-windows-gnu\stage2-tools\x86_64-pc-windows-gnu\release\deps\liblibgit2_sys-21cea83aba30ff6f.rlib(socket_stream.c.obj):socket_stream.c:(.text$socket_connect+0x187): undefined reference to `gai_strerrorA'
          collect2.exe: error: ld returned 1 exit status

@kennytm kennytm 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 Jan 7, 2018
@alexcrichton
Copy link
Member

Could this try updating just log so we can deal with that error later?

@kennytm
Copy link
Member

kennytm commented Jan 7, 2018

@alexcrichton We could deal with it later if we don't update cargo. (why the error is not caught in cargo's CI though?)

@alexcrichton
Copy link
Member

Yeah it's fine to not update Cargo here, the perf impact, in theory, is mostly in rustc. As to why it's not caught in Cargo no idea myself, they're inevitably different in some way and it's only a matter of time until you add something which exposes it.

Upgrade `log` to `0.4` in multiple crates.
@MaloJaffre
Copy link
Contributor Author

MaloJaffre commented Jan 7, 2018

I've rebased and removed the cargo submodule update.

@kennytm
Copy link
Member

kennytm commented Jan 7, 2018

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Jan 7, 2018

📌 Commit 3f073c4 has been approved by alexcrichton

@kennytm kennytm 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 7, 2018
@bors
Copy link
Contributor

bors commented Jan 7, 2018

⌛ Testing commit 3f073c4 with merge ee220da...

bors added a commit that referenced this pull request Jan 7, 2018
…chton

Try to fix a perf regression by updating log

Upgrade `log` to `0.4` in multiple crates ~and `cargo`~.
Cc: #47154.
@bors
Copy link
Contributor

bors commented Jan 7, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing ee220da to master...

@bors bors merged commit 3f073c4 into rust-lang:master Jan 7, 2018
@michaelwoerister
Copy link
Member

It seems to have worked: http://perf.rust-lang.org/compare.html?start=8724337c233f593e9961609d8b0855d0ec2357a0&end=ee220daca345302c3277befee2732b6b2a5a711c&stat=wall-time

Awesome! Thank you, @MaloJaffre!

@MaloJaffre MaloJaffre deleted the compiler-docs-regression branch January 10, 2018 12:23
@MaloJaffre
Copy link
Contributor Author

Does this needs to be beta-backported? (PR #46278 causing this regression was merged before the 1.24.0 beta release)

@kennytm kennytm added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 10, 2018
@michaelwoerister
Copy link
Member

Does this needs to be beta-backported?

Good catch! Yes, I think the performance difference is big enough to warrant a backport.

@michaelwoerister michaelwoerister added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 10, 2018
@alexcrichton
Copy link
Member

Tagging as accepted as this helps fix the regression

@alexcrichton alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 10, 2018
@MaloJaffre MaloJaffre mentioned this pull request Jan 10, 2018
bors added a commit that referenced this pull request Jan 10, 2018
[beta] Backports

Cherry-picked (cleanly) into beta:
- #46916
- #47161
- #47208
- #47269
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 11, 2018
@nikomatsakis
Copy link
Contributor

So I currently get log output like this:

DEBUG:<unknown>: run_on_mir: /home/nmatsakis/tmp/issue-47470.rs:12:5: 15:6
DEBUG:<unknown>: check_stmt: StorageLive(_2)
DEBUG:<unknown>: check_stmt: _2 = const 42u32
DEBUG:<unknown>: check_stmt: StorageLive(_3)
DEBUG:<unknown>: check_stmt: _3 = &'_#4r _2
DEBUG:<unknown>: check_stmt: _0 = &'_#5r (*_3)

in particular, the <unknown> parts used to be crate/module paths. Any idea why this is? Could it be related to upgrading log?

@alexcrichton
Copy link
Member

@KodrAus hm do you know why that may be occurring?

@alexcrichton
Copy link
Member

Ah I see, it's because we're using env_logger which links to the older version of log. @KodrAus out of curiosity, do you know when env_logger 0.5 will be ready which links to log 0.4?

@KodrAus
Copy link
Contributor

KodrAus commented Jan 16, 2018

@alexcrichton I'll get rust-cli/env_logger#53 merged and then push 0.5 along with some PRs for rustc and cargo and any other repos folks would like updated 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants