Skip to content

liballoc: mark str.to_owned() and String::from(&str) as #[inline]. #54613

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

Merged
merged 1 commit into from
Oct 9, 2018

Conversation

matthiaskrgr
Copy link
Member

Fixes #53681

@rust-highfive
Copy link
Contributor

r? @bluss

(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 Sep 27, 2018
@matthiaskrgr
Copy link
Member Author

I didn't notice any benchmarks regressing and it size increase of binaries seem to be below 1% in average but perhaps this should get a perf run anyway, just to be extra sure.

@nagisa
Copy link
Member

nagisa commented Sep 27, 2018

@bors try

@bors
Copy link
Collaborator

bors commented Sep 27, 2018

⌛ Trying commit d24070b with merge 02d7f80...

bors added a commit that referenced this pull request Sep 27, 2018
liballoc: mark str.to_owned() and String::from(&str) as #[inline].

Fixes #53681
@bors
Copy link
Collaborator

bors commented Sep 27, 2018

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

@nagisa
Copy link
Member

nagisa commented Sep 27, 2018

@rust-timer build 02d7f80

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@Mark-Simulacrum
Copy link
Member

@rust-timer build 02d7f80

@rust-timer
Copy link
Collaborator

Success: Queued 02d7f80 with parent e999ebd, comparison URL.

@matthiaskrgr
Copy link
Member Author

Hmm, still no perf results? 🤔

@Mark-Simulacrum
Copy link
Member

It looks like something went wrong during collection; @bors try

@nagisa
Copy link
Member

nagisa commented Sep 30, 2018

@rust-timer build 02d7f80

@rust-timer
Copy link
Collaborator

Success: Queued 02d7f80 with parent e999ebd, comparison URL.

@nagisa
Copy link
Member

nagisa commented Sep 30, 2018

Lets see if we can repeat rust-timer builds with results of previous try builds :)

@nagisa
Copy link
Member

nagisa commented Sep 30, 2018

Seems like not.

@bors retry

@bors
Copy link
Collaborator

bors commented Sep 30, 2018

⌛ Trying commit d24070b with merge 9aac7ca572e8ba96e99249a1a9d026331374f228...

@bors
Copy link
Collaborator

bors commented Sep 30, 2018

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

@nagisa
Copy link
Member

nagisa commented Sep 30, 2018

@rust-timer build 9aac7ca572e8ba96e99249a1a9d026331374f228

@rust-timer
Copy link
Collaborator

Success: Queued 9aac7ca572e8ba96e99249a1a9d026331374f228 with parent 6310be4, comparison URL.

@nagisa
Copy link
Member

nagisa commented Sep 30, 2018

Overall a very slight increase in number of executed instructions, but that says very little.

@TimNN
Copy link
Contributor

TimNN commented Oct 9, 2018

Ping from triage @bluss / @nagisa: What is the status of this PR?

@nagisa
Copy link
Member

nagisa commented Oct 9, 2018

@bors r+

I’ll r+ this for the following reasons: while the slight increase in number of executed instructions exists, I believe it is a incorrect statistic to look at here. Inlining can easily go one way or the other on that, while also enabling other optimisations (like eliding allocations) that are important for naive code to be more efficient. The specific case in the code(expected) provided in the issue is a prime example of such enablement – LLVM elides the code for the allocation altogether, because the information necessary to do so is available in that context.

@bors
Copy link
Collaborator

bors commented Oct 9, 2018

📌 Commit d24070b has been approved by nagisa

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2018
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 9, 2018
@bors
Copy link
Collaborator

bors commented Oct 9, 2018

⌛ Testing commit d24070b with merge 96cafc5...

bors added a commit that referenced this pull request Oct 9, 2018
liballoc: mark str.to_owned() and String::from(&str) as #[inline].

Fixes #53681
@bors
Copy link
Collaborator

bors commented Oct 9, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nagisa
Pushing 96cafc5 to master...

@bors bors merged commit d24070b into rust-lang:master Oct 9, 2018
@matthiaskrgr matthiaskrgr deleted the string_from_inline_53681 branch February 29, 2020 14:13
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.

8 participants