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

Add internal io::Error::new_const to avoid allocations. #83353

Merged
merged 5 commits into from
Mar 24, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Mar 21, 2021

This makes it possible to have a io::Error containing a message with zero allocations, and uses that everywhere to avoid the three allocations involved in io::Error::new(kind, "message").

The function signature isn't perfect, because it needs a reference to the &str. So for now, this is just a pub(crate) function. Later, we'll be able to use fn new_const<MSG: &'static str>(kind: ErrorKind) to make that a bit better. (Then we'll also be able to use some ZST trickery if that would result in more efficient code.)

See #83352

@rust-highfive
Copy link
Collaborator

r? @kennytm

(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 Mar 21, 2021
@m-ou-se m-ou-se added T-libs Relevant to the library team, which will review and decide on the PR/issue. I-heavy Issue: Problems and improvements with respect to binary size of generated code. and removed I-heavy Issue: Problems and improvements with respect to binary size of generated code. labels Mar 21, 2021
library/std/src/io/error.rs Outdated Show resolved Hide resolved
library/std/src/io/error.rs Outdated Show resolved Hide resolved
@the8472
Copy link
Member

the8472 commented Mar 21, 2021

There also is a typo in the PR title.

@jyn514 jyn514 changed the title Add internal io::Error::new_const tot avoid allocations. Add internal io::Error::new_const to avoid allocations. Mar 21, 2021
@jyn514
Copy link
Member

jyn514 commented Mar 21, 2021

This seems like a good use case for specialization.

@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 21, 2021

This seems like a good use case for specialization.

We spent quite a while with several people trying to use specialization here. If you find a way to do this in a nicer way with specialization, let me know. ^^

Co-authored-by: the8472 <the8472@users.noreply.github.com>
@mystor
Copy link
Contributor

mystor commented Mar 21, 2021

This seems like a good use case for specialization.

Unfortunately the signature of io::Error::new takes an E: Into<Box<dyn Error + Send + Sync>> with no other bounds, meaning &'a str is a valid argument for any 'a. It's impossible to specialize on lifetimes, so io::Error::new can never be specialized for &'static str.

@nagisa
Copy link
Member

nagisa commented Mar 23, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 23, 2021

📌 Commit 6bbcc5b has been approved by nagisa

@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 Mar 23, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 23, 2021
Add internal io::Error::new_const to avoid allocations.

This makes it possible to have a io::Error containing a message with zero allocations, and uses that everywhere to avoid the *three* allocations involved in `io::Error::new(kind, "message")`.

The function signature isn't perfect, because it needs a reference to the `&str`. So for now, this is just a `pub(crate)` function. Later, we'll be able to use `fn new_const<MSG: &'static str>(kind: ErrorKind)` to make that a bit better. (Then we'll also be able to use some ZST trickery if that would result in more efficient code.)

See rust-lang#83352
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 23, 2021
Add internal io::Error::new_const to avoid allocations.

This makes it possible to have a io::Error containing a message with zero allocations, and uses that everywhere to avoid the *three* allocations involved in `io::Error::new(kind, "message")`.

The function signature isn't perfect, because it needs a reference to the `&str`. So for now, this is just a `pub(crate)` function. Later, we'll be able to use `fn new_const<MSG: &'static str>(kind: ErrorKind)` to make that a bit better. (Then we'll also be able to use some ZST trickery if that would result in more efficient code.)

See rust-lang#83352
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2021
Rollup of 9 pull requests

Successful merges:

 - rust-lang#83051 (Sidebar trait items order)
 - rust-lang#83313 (Only enable assert_dep_graph when query-dep-graph is enabled.)
 - rust-lang#83353 (Add internal io::Error::new_const to avoid allocations.)
 - rust-lang#83391 (Allow not emitting `uwtable` on Android)
 - rust-lang#83392 (Change `-W help` to display edition level.)
 - rust-lang#83393 (Codeblock tooltip position)
 - rust-lang#83399 (rustdoc: Record crate name instead of using `None`)
 - rust-lang#83405 (Slight visual improvements to warning boxes in the docs)
 - rust-lang#83415 (Remove unnecessary `Option` wrapping around `Crate.module`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a42e62f into rust-lang:master Mar 24, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 24, 2021
@m-ou-se m-ou-se deleted the io-error-avoid-alloc branch March 24, 2021 10:38
@Frago9876543210
Copy link

Frago9876543210 commented Mar 26, 2021

Another problem is that some crates still depends on Error::new(). So, this PR reduces only stdlib allocations. The only solution one can think of is a smart-pointer like Cow. I.e. need to be done so that Into<Box<dyn Error + ...>> actually allocates memory in needed cases.

pub enum ThinBox<'a, T> {
    Borrowed(&'a T),
    Owned(Box<T>),
}

fn main() {
    dbg!(std::mem::size_of::<ThinBox<'static, &'static str>>()); //16
}

However, now it is not clear how not to break Error::into_inner().

impl Error {
    pub fn into_inner(self) -> Option<Box<dyn Error + Send + Sync>> { ... }
}

cc @m-ou-se

@m-ou-se
Copy link
Member Author

m-ou-se commented Mar 26, 2021

Another problem is that some crates still depends on Error::new().

@Frago9876543210 We can't fix std::io::Error::new(kind, "some_str") to avoid allocations, since that's the exact same function as in std::io::Error::new(kind, tmp) where tmp is a &'a str that doesn't live for 'static. That works because the entire string is copied onto the heap. There's no way to specialize on lifetimes, so in order to support e.g. std::io::Error::new(kind, &format!("{}", 123)), the version with a string literal will have to keep doing the same.

The only way to provide this to other crates is by adding a new function (or new type), which requires crates to update their code. We could make the new_const introduced here public, or wait until const &str generics are a thing if we want to use that syntax instead.

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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants