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.

Sign up for GitHub

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

Jump to bottom

Enable default inlining in platform intrinsics #52426

Merged
merged 1 commit into from
Jul 18, 2018
Merged

Enable default inlining in platform intrinsics #52426

merged 1 commit into from
Jul 18, 2018

Conversation

ljedrz
Copy link
Contributor

@ljedrz ljedrz commented Jul 16, 2018

Since #28273 has been fixed for quite some time, it might be a good idea to return to default inlining in platform intrinsics.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(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 16, 2018
@nikomatsakis
Copy link
Contributor

Heh, good catch. Shall we do a performance run?

@nikomatsakis
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Jul 16, 2018

⌛ Trying commit e7f63f1 with merge 278242e1492c09c1dfe81b7a892e6a16367445f2...

@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 16, 2018 •
edited
Loading

I think it might be a good idea to run it by perf, because the differences that called for this change (i.e. disabling inlining) were pretty big.

@hanna-kruppe
Copy link
Contributor

Not that perf won't measure the time it took to build rustc and the sysroot, which is where the big slowdown was. It'll only tell us whether the change has any benefit for compiling other code with rustc, i.e., whether potentially inlining these functions improved rustc's running time, which is unrelated.

@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 16, 2018

Thanks for the correction, I'm still pretty new to this; what would be the best way to test the difference in performance for this sort of change, then?

@hanna-kruppe
Copy link
Contributor

I'm not 100% sure but since the crate is pretty stand-alone, it seems like you could just try building it (only the crate, not the whole compiler) with cargo build --release with and without your patch and see if there's any difference?

@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 16, 2018

Cool; I'll do that soon and provide some numbers.

@nikomatsakis
Copy link
Contributor

@rkruppe so this produced a slowdown that was specific to compiling rustc?

@hanna-kruppe
Copy link
Contributor

More specifically to compiling libplatform_intrinsics, yes. Unless I've been reading #28273 wrong the entire time since it was filed, including just now when I went back to double-check.

@bors
Copy link
Contributor

bors commented Jul 16, 2018

☀️ Test successful - status-travis
State: approved= try=True

@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 16, 2018

I compiled a few times locally and there's no regression, at least on Windows 10 x64.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 17, 2018

📌 Commit e7f63f1 has been approved by nikomatsakis

@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 17, 2018
@bors
Copy link
Contributor

bors commented Jul 18, 2018

⌛ Testing commit e7f63f1 with merge 38168a7...

bors added a commit that referenced this pull request Jul 18, 2018
@bors
Auto merge of #52426 - ljedrz:#28273_cleanup, r=nikomatsakis
38168a7 
Enable default inlining in platform intrinsics

Since [#28273](#28273) has been fixed for quite some time, it might be a good idea to return to default inlining in platform intrinsics.
@bors
Copy link
Contributor

bors commented Jul 18, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 38168a7 to master...

@bors bors merged commit e7f63f1 into rust-lang:master Jul 18, 2018
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #52426!

Tested on commit 38168a7.
Direct link to PR: #52426

💔 book on linux: test-pass → test-fail (cc @carols10cents @steveklabnik, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jul 18, 2018
@rust-highfive
📣 Toolstate changed by rust-lang/rust#52426!
67e8193 
Tested on commit rust-lang/rust@38168a7.
Direct link to PR: <rust-lang/rust#52426>

💔 book on linux: test-pass → test-fail (cc @carols10cents @steveklabnik, @rust-lang/infra).
@ljedrz ljedrz deleted the #28273_cleanup branch July 18, 2018 11:26
@kennytm
Copy link
Member

kennytm commented Jul 18, 2018 •
edited
Loading

Not sure if spurious.

[00:49:01] doc tests for: /checkout/src/doc/book/second-edition/src/ch07-03-importing-names-with-use.md
[00:49:01] 
[00:49:01] running 10 tests
[00:49:01] test /checkout/src/doc/book/second-edition/src/ch07-03-importing-names-with-use.md - _::Referring_to_Names_in_Different_Modules (line 9) ... ok
[00:49:01] test /checkout/src/doc/book/second-edition/src/ch07-03-importing-names-with-use.md - _::Referring_to_Names_in_Different_Modules::Bringing_Names_into_Scope_with_the_ (line 64) ... ok
[00:49:01] test /checkout/src/doc/book/second-edition/src/ch07-03-importing-names-with-use.md - _::Referring_to_Names_in_Different_Modules::Using_ (line 142) ... ignored
[00:49:01] test /checkout/src/doc/book/second-edition/src/ch07-03-importing-names-with-use.md - _::Referring_to_Names_in_Different_Modules::Bringing_Names_into_Scope_with_the_ (line 37) ... ok
[00:49:01] test /checkout/src/doc/book/second-edition/src/ch07-03-importing-names-with-use.md - _::Referring_to_Names_in_Different_Modules::Using_ (line 210) ... ignored
[00:49:01] test /checkout/src/doc/book/second-edition/src/ch07-03-importing-names-with-use.md - _::Referring_to_Names_in_Different_Modules::Using_ (line 217) ... ignored
[00:49:01] test /checkout/src/doc/book/second-edition/src/ch07-03-importing-names-with-use.md - _::Referring_to_Names_in_Different_Modules::Bringing_All_Names_into_Scope_with_a_Glob (line 113) ... ok
[00:49:02] 
[00:49:02] 
[00:49:02] command did not execute successfully: "/checkout/obj/build/bootstrap/debug/rustdoc" "--test" "/checkout/src/doc/book/second-edition/src/ch07-03-importing-names-with-use.md" "--test-args" ""
[00:49:02] expected success, got: exit code: 1

Edit: Yes, spurious.

@RalfJung
Copy link
Member

The book got "fixed" again in #52364 so this seems spurious.

@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 18, 2018

What's the book?

@RalfJung
Copy link
Member

The Book

@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 18, 2018

Oh, the book. How did my commit fix it ❓

@RalfJung
Copy link
Member

RalfJung commented Jul 18, 2018 •
edited
Loading

Your one commit (this one here) broke it. Your other commit fixed it. We don't know why.^^

@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 18, 2018

That's most curious; please advise when you have found out, I'd like to not do that again 😆.

@RalfJung
Copy link
Member

If you break it again we'll just ask you to fix it again, like you did this time. :)

@ljedrz
Copy link
Contributor Author

ljedrz commented Jul 18, 2018

I guess I'll do my future contributions in pairs then ;).

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

@nikomatsakis nikomatsakis

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ljedrz @rust-highfive @nikomatsakis @bors @hanna-kruppe @kennytm @RalfJung