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

Enable Vec's calloc optimization for Option<NonZero> #85737

Merged
merged 1 commit into from
May 27, 2021

Conversation

scottmcm
Copy link
Member

@scottmcm scottmcm commented May 27, 2021

Someone on discord noticed that vec![None::<NonZeroU32>; N] wasn't getting the optimization, so here's a PR 🙃

We can certainly do this in the standard library because we know for sure this is ok, but I think it's also a necessary consequence of documented guarantees like those in https://doc.rust-lang.org/std/option/#representation and https://doc.rust-lang.org/core/num/struct.NonZeroU32.html

It feels weird to do this without adding a test, but I wasn't sure where that would belong. Is it worth adding codegen tests for these?

@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 May 27, 2021
@m-ou-se
Copy link
Member

m-ou-se commented May 27, 2021

This looks fine to me. It just makes me wonder if there's a better way we could do this for all types. Some kind of std::mem::is_just_zero_bits<T>(_: &T) -> bool with some magic implementation.

@bors r+

@bors
Copy link
Contributor

bors commented May 27, 2021

📌 Commit 04d34a9 has been approved by m-ou-se

@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 May 27, 2021
@m-ou-se m-ou-se assigned m-ou-se and unassigned kennytm May 27, 2021
@m-ou-se m-ou-se added A-collections Area: `std::collection` T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 27, 2021
@bors
Copy link
Contributor

bors commented May 27, 2021

⌛ Testing commit 04d34a9 with merge ea78d1e...

@bors
Copy link
Contributor

bors commented May 27, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing ea78d1e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 27, 2021
@bors bors merged commit ea78d1e into rust-lang:master May 27, 2021
@rustbot rustbot added this to the 1.54.0 milestone May 27, 2021
// `Option<num::NonZeroU32>` and similar have a representation guarantee that
// they're the same size as the corresponding `u32` type, as well as a guarantee
// that transmuting between `NonZeroU32` and `Option<num::NonZeroU32>` works.
// While the documentation officially makes in UB to transmute from `None`,
Copy link

@proudmuslim-dev proudmuslim-dev May 27, 2021

Choose a reason for hiding this comment

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

Shouldn't this be "makes it ub"?

Edit: I see this was already merged, I assume I was wrong

Copy link
Member

Choose a reason for hiding this comment

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

Oh, you're right. Feel free to send a PR if you want ^^'. But this doesn't appear in any public documentation/interface, so it doesn't matter that much.

@scottmcm scottmcm deleted the vec-calloc-option-nonzero branch May 27, 2021 22:14
@scottmcm
Copy link
Member Author

@m-ou-se I guess it depends on how much this wants to care about types with padding. Supporting (0_u8, 0_u16) would be hard, but adding an intrinsic for may_have_padding and just checking it for things that are known to definitely not have padding would be easy enough. (And the safe-transmute project might already be working on compiler code for that kind of check.)

@scottmcm
Copy link
Member Author

Wow, very weird spike!
image

Definitely revert this if it's causing that much of a check build regression, but I'm a bit skeptical this it could be that big of a problem. The PR where it drops back down again also don't look like it could have improved things that much. So maybe a runner hiccup.

@Mark-Simulacrum
Copy link
Member

Yeah, this was a runner hiccup I believe.

@klensy
Copy link
Contributor

klensy commented May 30, 2021

Yeah, this was a runner hiccup I believe.

Is there any logs, as part of benches isn't run at all (or not saved info about it, check lower part of bench list)?

@Mark-Simulacrum
Copy link
Member

There's some discussion on the rustc-perf repository, but there's really no need to investigate further in my opinion.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 30, 2021
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 30, 2021
@m-ou-se
Copy link
Member

m-ou-se commented May 31, 2021

I guess it depends on how much this wants to care about types with padding. Supporting (0_u8, 0_u16) would be hard, but adding an intrinsic for may_have_padding and just checking it for things that are known to definitely not have padding would be easy enough. (And the safe-transmute project might already be working on compiler code for that kind of check.)

Ideally, it should work for things with padding too. As long as an all-zero-bits value is an identical value, ignoring padding bits, it shoud return true. So a (0u8, 0u16) passed in with raw bytes 00 ff 00 00 should still return true because it's identical to 00 00 00 00, so 'calloc' can be used. No clue what it'd take to implement such an intrinsic, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: `std::collection` 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 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.

9 participants