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

Optimize extend and from_elem #93

Merged
merged 3 commits into from
May 7, 2018
Merged

Conversation

eira-fransham
Copy link
Contributor

@eira-fransham eira-fransham commented May 7, 2018

This increases the amount of unsafe code, and so should be carefully checked. However, the use of unsafe code is quite benign and depends only on the invariant that grow always succeeds (which is depended on other places in the codebase)

Benchcmp results:

 name                     preopt.bench ns/iter  postopt.bench ns/iter  diff ns/iter   diff %  speedup 
 bench_extend             277                   89                             -188  -67.87%   x 3.11  
 bench_macro_from_elem    214                   61                             -153  -71.50%   x 3.51 

I took a crack at this after noticing how awful the generated assembly for SmallVec::from_elem is even for really simple types like integers. It's still not great but it at least now for the case of Copy types it gets autovectorised. Probably someone could make it even better if they spent more time on it.


This change is Reviewable

@eira-fransham
Copy link
Contributor Author

A comparison of the generated assembly for a simple (but probably common) case is here

@eira-fransham
Copy link
Contributor Author

The reasoning behind using std::vec::from_elem/vec! over .extend(iter::repeat(..).take(..)) is that u8 and integer 0 have a fast path in vec::from_elem using ptr::write_bytes and calloc, respectively. We could conceivably also use ptr::write_bytes on u8 and 0 when the vec would be inline but that would require a nightly feature (specialisation) and would only be noticeable on huge inline arrays.

@mbrubeck
Copy link
Collaborator

mbrubeck commented May 7, 2018

@bors-servo r+

Thanks!

@bors-servo
Copy link
Contributor

📌 Commit 401fb09 has been approved by mbrubeck

@bors-servo
Copy link
Contributor

⌛ Testing commit 401fb09 with merge 8c9f6c8...

bors-servo pushed a commit that referenced this pull request May 7, 2018
Optimize `extend` and `from_elem`

This increases the amount of unsafe code, and so should be carefully checked. However, the use of unsafe code is quite benign and depends only on the invariant that `grow` always succeeds (which is depended on other places in the codebase)

Benchcmp results:

```
 name                     preopt.bench ns/iter  postopt.bench ns/iter  diff ns/iter   diff %  speedup
 bench_extend             277                   89                             -188  -67.87%   x 3.11
 bench_macro_from_elem    214                   61                             -153  -71.50%   x 3.51
```

I took a crack at this after noticing how awful the generated assembly for `SmallVec::from_elem` is even for really simple types like integers. It's still not great but it at least now for the case of `Copy` types it gets autovectorised. Probably someone could make it even better if they spent more time on it.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/93)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - status-travis
Approved by: mbrubeck
Pushing 8c9f6c8 to master...

@bors-servo bors-servo merged commit 401fb09 into servo:master May 7, 2018
@mbrubeck
Copy link
Collaborator

mbrubeck commented May 7, 2018

If you have a project that needs this, let us know and we can publish a release right away. (If not, it'll get published sooner or later, whenever the next release happens.)

mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this pull request Jul 19, 2018
Changes in this release:

* Fix possible double-free in `insert_many` when passed an iterator that
  panics in `next` (servo#103)
* Add a new `union` feature (disabled by default) that reduces the size
  of the SmallVec struct (servo#94)
* Improve performance of `extend` and `from_elem` (servo#93)
* Improve performance of `drop` (servo#100)
* Update dev-dependency on `bincode` (servo#102)
* Update to build without `std` on current Rust nightly (servo#104)
* Additional benchmarks (servo#95, servo#97).
mbrubeck added a commit to mbrubeck/rust-smallvec that referenced this pull request Jul 19, 2018
Changes in this release:

* Fix possible double-free in `insert_many` when passed an iterator that
  panics in `next` (servo#103)
* Add a new `union` feature (disabled by default) that reduces the size
  of the SmallVec struct (servo#94)
* Improve performance of `extend` and `from_elem` (servo#93)
* Improve performance of `drop` (servo#100)
* Update dev-dependency on `bincode` (servo#102)
* Update to build without `std` on current Rust nightly (servo#104)
* Additional benchmarks (servo#95, servo#97).
@mbrubeck mbrubeck mentioned this pull request Jul 19, 2018
bors-servo pushed a commit that referenced this pull request Jul 19, 2018
Version 0.6.3

Changes in this release:

* Fix possible double-free in `insert_many` when passed an iterator that panics in `next` (#103)
* Add a new `union` feature (disabled by default) that reduces the size of the SmallVec struct (#94)
* Improve performance of `extend` and `from_elem` (#93)
* Improve performance of `drop` (#100)
* Update to build without `std` feature on current Rust nightly (#104)
* Additional benchmarks (#95, #97)
* Update dev-dependency on `bincode` (#102)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-smallvec/105)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants