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

Hide allocator details from TryReserveError #87408

Merged
merged 1 commit into from
Aug 7, 2021

Conversation

kornelski
Copy link
Contributor

@kornelski kornelski commented Jul 23, 2021

I think there's no need for TryReserveError to carry detailed information, but I wouldn't want that issue to delay stabilization of the try_reserve feature.

So I'm proposing to stabilize try_reserve with a TryReserveError as an opaque structure, and if needed, expose error details later.

This PR moves the enum to an unstable inner TryReserveErrorKind that lives under a separate feature flag. TryReserveErrorKind could possibly be left as an implementation detail forever, and the TryReserveError get methods such as allocation_size() -> Option<usize> or layout() -> Option<Layout> instead, or the details could be dropped completely to make try-reserve errors just a unit struct, and thus smaller and cheaper.

@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 Jul 23, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@kennytm
Copy link
Member

kennytm commented Jul 24, 2021

🤔 i'll defer this to T-libs-api to consider.

r? @rust-lang/libs-api

@Amanieu Amanieu added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 24, 2021
Copy link
Member

@yaahc yaahc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love how every match against a TryReserveError needs an explicit map_err to get the kind now before matching, but as far as I can tell it wouldn't be backwards incompatible to switch the opaque struct back to an enum in the future to enable direct convenient matching once we're confident we know the representation we want internally.

+1 from me but I'll wait until after today's libs-api team meeting before approving since it's already nominated.

@yaahc
Copy link
Member

yaahc commented Aug 6, 2021

Everyone seemed amenable to this change in the libs-api meeting, though we didn't go into a particularly detailed discussion about it. The expectation being that such a discussion would happen during follow up stabilization discussions.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 6, 2021

📌 Commit a294aa8 has been approved by yaahc

@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 Aug 6, 2021
@bors
Copy link
Contributor

bors commented Aug 7, 2021

⌛ Testing commit a294aa8 with merge 996ff2e...

@bors
Copy link
Contributor

bors commented Aug 7, 2021

☀️ Test successful - checks-actions
Approved by: yaahc
Pushing 996ff2e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 7, 2021
@bors bors merged commit 996ff2e into rust-lang:master Aug 7, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 7, 2021
@kornelski kornelski deleted the try_reserve_error branch August 7, 2021 11:27
@pnkfelix
Copy link
Member

Just a quick heads up: For some reason, this PR seems to have regressed the html5ever-opt benchmark: 4.24% more instructions:u, and 8.16% more max-rss.

I'm not sure why this PR would cause that change, and it seems somewhat isolated to html5ever-opt. Skimming over the max-rss graphs, we cna see that html5-ever-opt is somewhat noisy, but it seems like a ~5% bump in max-rss can be potentially attributed to this PR.

(At the same time, it isn't a broad regression across many benchmarks. So it probably isn't worth investing much time looking into this. I just wanted to post a heads up.)

@the8472
Copy link
Member

the8472 commented Aug 11, 2021

#87843 might address the regression.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2021
TryReserveErrorKind tests and inline

A small follow-up to rust-lang#87408
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants