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

Optimise vec![false; N] to zero-alloc #58628

Merged
merged 1 commit into from Feb 23, 2019

Conversation

Projects
None yet
7 participants
@RReverser
Copy link
Contributor

RReverser commented Feb 22, 2019

Nowadays booleans have a well-defined representation, so there is no reason not to optimise their allocation.

Optimise vec![false; N] to zero-alloc
Nowadays booleans have a well-defined representation, so there is no reason not to optimise their allocation.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 22, 2019

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Feb 22, 2019

This makes sense to me and I agree the change is correct, but should it perhaps have a test somewhere? Just a boring one that vec![false; 100][50] == false or something...

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Feb 22, 2019

I think we could add a few more of these:

  • Option<&T>::None (even where T: ?Sized, since in the none case we don't care about the fat pointer part)
  • Option<NonZero<T>>::None
  • Option<NonZeroU8>::None and friends
  • Wrapping<T> for T: IsZero
  • arrays and tuples of IsZero things (probably leave those for the future or just impl a few simple ones)

Also the u8 specialization for SpecFromElem could be implemented for

  • i8
  • bool
  • Option<bool>
  • Option<NonZeroU8> and Option<NonZeroI8>
  • Wrapping<i8> and Wrapping<u8>

Is this going overboard? 😆

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Feb 22, 2019

At the end of this PR, let's do a perf test :)

@Centril

This comment has been minimized.

Copy link
Contributor

Centril commented Feb 22, 2019

Is this going overboard? 😆

Seems fine, but please leave comments re. what representation assumptions are actually a stable part of the language and which are just made because this is libstd...

@RReverser

This comment has been minimized.

Copy link
Contributor Author

RReverser commented Feb 22, 2019

@oli-obk As for u8 specialisation, I'm not sure it's necessary these days since even without it I get a pretty much same assembly using zero-alloc for zero values and memset for non-zero ones, so I think it can be removed but as it's a more invasive change and requires more testing, decided not to touch it for now.

As for the other types, sure, they could be added, but, just like above, tried to keep scope of changes minimal.

I can add them here, but they don't seem too common in vectors, and I'm more interested in a generic solution that would be applicable to newtypes and such. Right now wrapping any of these values into a newtype breaks optimisation.

Unfortunately, the only way to make this generic, as mentioned on the original PR, would be to read bytes of type and check if they are all zero, but this is hard to do without reading the padding too, and that one is currently defined as UB. Hence, it will require waiting for a well-defined memory model.

@RReverser

This comment has been minimized.

Copy link
Contributor Author

RReverser commented Feb 22, 2019

the only way

Actually, I juts recalled that I had another horrible idea for making this work on newtypes and any other types, although I'm not 100% sure if it's sound, but it shouldn't be UB.

I can give it a try in this same PR if you'd like to take a look?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Feb 22, 2019

I can give it a try in this same PR if you'd like to take a look?

Now you've made me curious, yes please

As for the other types, sure, they could be added, but, just like above, tried to keep scope of changes minimal.

Makes sense.

@RReverser

This comment has been minimized.

Copy link
Contributor Author

RReverser commented Feb 22, 2019

sigh looks like my idea, while works with LLVM, is currently also unsound / UB in Rust terms, so nevermind.

As such, I guess I'd prefer to limit this PR to its current scope or could add more types as requested.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Feb 22, 2019

As such, I guess I'd prefer to limit this PR to its current scope or could add more types as requested.

Ok, it's not necessary to do these now, as you said, they are rather rare.

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 22, 2019

📌 Commit 9f58c5f has been approved by oli-obk

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Feb 22, 2019

At the end of this PR, let's do a perf test :)

It's not gonna show up in perf I believe. Even if we are using vec![false; n] somewhere in rustc, it's going to be negligible.

@RReverser

This comment has been minimized.

Copy link
Contributor Author

RReverser commented Feb 22, 2019

It's not gonna show up in perf I believe. Even if we are using vec![false; n] somewhere in rustc, it's going to be negligible.

I think he meant a generic benchmark results. Although, since boolean is essentially a u8 in memory terms, I believe these results should be the same as for initial PR (except the default allocator has changed in the meanwhile):

Before this PR:

test bench_big    ... bench:     231,338 ns/iter (+/- 97,246)
test bench_medium ... bench:          34 ns/iter (+/- 1)
test bench_small  ... bench:          24 ns/iter (+/- 3)

After this PR:

test bench_big    ... bench:         853 ns/iter (+/- 47)
test bench_medium ... bench:          37 ns/iter (+/- 2)
test bench_small  ... bench:          25 ns/iter (+/- 1)

(c) #40409 (comment)

Centril added a commit to Centril/rust that referenced this pull request Feb 23, 2019

Rollup merge of rust-lang#58628 - RReverser:optimise-vec-false, r=oli…
…-obk

Optimise vec![false; N] to zero-alloc

Nowadays booleans have a well-defined representation, so there is no reason not to optimise their allocation.

Centril added a commit to Centril/rust that referenced this pull request Feb 23, 2019

Rollup merge of rust-lang#58628 - RReverser:optimise-vec-false, r=oli…
…-obk

Optimise vec![false; N] to zero-alloc

Nowadays booleans have a well-defined representation, so there is no reason not to optimise their allocation.

bors added a commit that referenced this pull request Feb 23, 2019

Auto merge of #58665 - Centril:rollup, r=Centril
Rollup of 16 pull requests

Successful merges:

 - #58122 (RangeInclusive internal iteration performance improvement.)
 - #58199 (Add better error message for partial move)
 - #58216 ( Set secure flags when opening a named pipe on Windows )
 - #58227 (Updated RELEASES.md for 1.33.0)
 - #58353 (Check the Self-type of inherent associated constants)
 - #58453 (SGX target: fix panic = abort)
 - #58454 (Refactor Windows stdio and remove stdin double buffering )
 - #58476 (Remove `LazyTokenStream`.)
 - #58526 (Special suggestion for illegal unicode curly quote pairs)
 - #58595 (Turn duration consts into associated consts)
 - #58609 (Allow Self::Module to be mutated.)
 - #58628 (Optimise vec![false; N] to zero-alloc)
 - #58642 (rustdoc: support methods on primitives in intra-doc links)
 - #58643 (Don't generate minification variables if minification disabled)
 - #58648 (Update tests to account for cross-platform testing and miri.)
 - #58654 (Do not underflow after resetting unmatched braces count)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Feb 23, 2019

Rollup merge of rust-lang#58628 - RReverser:optimise-vec-false, r=oli…
…-obk

Optimise vec![false; N] to zero-alloc

Nowadays booleans have a well-defined representation, so there is no reason not to optimise their allocation.

bors added a commit that referenced this pull request Feb 23, 2019

Auto merge of #58666 - Centril:rollup, r=Centril
Rollup of 15 pull requests

Successful merges:

 - #58122 (RangeInclusive internal iteration performance improvement.)
 - #58199 (Add better error message for partial move)
 - #58216 ( Set secure flags when opening a named pipe on Windows )
 - #58227 (Updated RELEASES.md for 1.33.0)
 - #58353 (Check the Self-type of inherent associated constants)
 - #58453 (SGX target: fix panic = abort)
 - #58476 (Remove `LazyTokenStream`.)
 - #58526 (Special suggestion for illegal unicode curly quote pairs)
 - #58595 (Turn duration consts into associated consts)
 - #58609 (Allow Self::Module to be mutated.)
 - #58628 (Optimise vec![false; N] to zero-alloc)
 - #58642 (rustdoc: support methods on primitives in intra-doc links)
 - #58643 (Don't generate minification variables if minification disabled)
 - #58648 (Update tests to account for cross-platform testing and miri.)
 - #58654 (Do not underflow after resetting unmatched braces count)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Feb 23, 2019

Rollup merge of rust-lang#58628 - RReverser:optimise-vec-false, r=oli…
…-obk

Optimise vec![false; N] to zero-alloc

Nowadays booleans have a well-defined representation, so there is no reason not to optimise their allocation.

Centril added a commit to Centril/rust that referenced this pull request Feb 23, 2019

Rollup merge of rust-lang#58628 - RReverser:optimise-vec-false, r=oli…
…-obk

Optimise vec![false; N] to zero-alloc

Nowadays booleans have a well-defined representation, so there is no reason not to optimise their allocation.

bors added a commit that referenced this pull request Feb 23, 2019

Auto merge of #58669 - Centril:rollup, r=Centril
Rollup of 16 pull requests

Successful merges:

 - #58100 (Transition librustdoc to Rust 2018)
 - #58122 (RangeInclusive internal iteration performance improvement.)
 - #58199 (Add better error message for partial move)
 - #58227 (Updated RELEASES.md for 1.33.0)
 - #58353 (Check the Self-type of inherent associated constants)
 - #58453 (SGX target: fix panic = abort)
 - #58476 (Remove `LazyTokenStream`.)
 - #58526 (Special suggestion for illegal unicode curly quote pairs)
 - #58595 (Turn duration consts into associated consts)
 - #58609 (Allow Self::Module to be mutated.)
 - #58628 (Optimise vec![false; N] to zero-alloc)
 - #58643 (Don't generate minification variables if minification disabled)
 - #58648 (Update tests to account for cross-platform testing and miri.)
 - #58654 (Do not underflow after resetting unmatched braces count)
 - #58658 (Add expected/provided byte alignments to validation error message)
 - #58667 (Reduce Miri-related Code Repetition `like (n << amt) >> amt`)

Failed merges:

r? @ghost

bors added a commit that referenced this pull request Feb 23, 2019

Auto merge of #58669 - Centril:rollup, r=Centril
Rollup of 16 pull requests

Successful merges:

 - #58100 (Transition librustdoc to Rust 2018)
 - #58122 (RangeInclusive internal iteration performance improvement.)
 - #58199 (Add better error message for partial move)
 - #58227 (Updated RELEASES.md for 1.33.0)
 - #58353 (Check the Self-type of inherent associated constants)
 - #58453 (SGX target: fix panic = abort)
 - #58476 (Remove `LazyTokenStream`.)
 - #58526 (Special suggestion for illegal unicode curly quote pairs)
 - #58595 (Turn duration consts into associated consts)
 - #58609 (Allow Self::Module to be mutated.)
 - #58628 (Optimise vec![false; N] to zero-alloc)
 - #58643 (Don't generate minification variables if minification disabled)
 - #58648 (Update tests to account for cross-platform testing and miri.)
 - #58654 (Do not underflow after resetting unmatched braces count)
 - #58658 (Add expected/provided byte alignments to validation error message)
 - #58667 (Reduce Miri-related Code Repetition `like (n << amt) >> amt`)

Failed merges:

r? @ghost

@bors bors merged commit 9f58c5f into rust-lang:master Feb 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.