Skip to content

Switch from SmallVec to 100% safe TinyVec #54

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.

Sign up for GitHub

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

Jump to bottom
Merged
Manishearth merged 4 commits into from
Mar 16, 2020
Merged

Switch from SmallVec to 100% safe TinyVec #54

Manishearth merged 4 commits into from
Mar 16, 2020

Conversation

@Shnatsel
Copy link
Contributor

@Shnatsel Shnatsel commented Mar 15, 2020

Replaces uses of SmallVec with TinyVec, which is the same idea implemented in 100% safe code.

Performance is generally in line with SmallVec in this case, with some benchmarks being slightly slower and some slightly faster:

SmallVec 1.1.0

test bench_is_nfc_ascii                      ... bench:          15 ns/iter (+/- 0)
test bench_is_nfc_normalized                 ... bench:          26 ns/iter (+/- 0)
test bench_is_nfc_not_normalized             ... bench:         455 ns/iter (+/- 7)
test bench_is_nfc_stream_safe_ascii          ... bench:          14 ns/iter (+/- 0)
test bench_is_nfc_stream_safe_normalized     ... bench:          39 ns/iter (+/- 0)
test bench_is_nfc_stream_safe_not_normalized ... bench:         526 ns/iter (+/- 9)
test bench_is_nfd_ascii                      ... bench:          17 ns/iter (+/- 3)
test bench_is_nfd_normalized                 ... bench:          43 ns/iter (+/- 0)
test bench_is_nfd_not_normalized             ... bench:          15 ns/iter (+/- 0)
test bench_is_nfd_stream_safe_ascii          ... bench:          17 ns/iter (+/- 3)
test bench_is_nfd_stream_safe_normalized     ... bench:          72 ns/iter (+/- 1)
test bench_is_nfd_stream_safe_not_normalized ... bench:          15 ns/iter (+/- 0)
test bench_nfc_ascii                         ... bench:         557 ns/iter (+/- 6)
test bench_nfc_long                          ... bench:     235,422 ns/iter (+/- 2,928)
test bench_nfd_ascii                         ... bench:         325 ns/iter (+/- 6)
test bench_nfd_long                          ... bench:     133,196 ns/iter (+/- 1,167)
test bench_nfkc_ascii                        ... bench:         557 ns/iter (+/- 19)
test bench_nfkc_long                         ... bench:     252,428 ns/iter (+/- 2,076)
test bench_nfkd_ascii                        ... bench:         300 ns/iter (+/- 3)
test bench_nfkd_long                         ... bench:     137,857 ns/iter (+/- 1,435)
test bench_streamsafe_adversarial            ... bench:         471 ns/iter (+/- 7)
test bench_streamsafe_ascii                  ... bench:          81 ns/iter (+/- 1)

TinyVec 0.3.2

test bench_is_nfc_ascii                      ... bench:          14 ns/iter (+/- 0)
test bench_is_nfc_normalized                 ... bench:          26 ns/iter (+/- 0)
test bench_is_nfc_not_normalized             ... bench:         440 ns/iter (+/- 3)
test bench_is_nfc_stream_safe_ascii          ... bench:          14 ns/iter (+/- 0)
test bench_is_nfc_stream_safe_normalized     ... bench:          38 ns/iter (+/- 0)
test bench_is_nfc_stream_safe_not_normalized ... bench:         524 ns/iter (+/- 5)
test bench_is_nfd_ascii                      ... bench:          16 ns/iter (+/- 2)
test bench_is_nfd_normalized                 ... bench:          42 ns/iter (+/- 0)
test bench_is_nfd_not_normalized             ... bench:          14 ns/iter (+/- 0)
test bench_is_nfd_stream_safe_ascii          ... bench:          16 ns/iter (+/- 0)
test bench_is_nfd_stream_safe_normalized     ... bench:          53 ns/iter (+/- 0)
test bench_is_nfd_stream_safe_not_normalized ... bench:          15 ns/iter (+/- 0)
test bench_nfc_ascii                         ... bench:         595 ns/iter (+/- 8)
test bench_nfc_long                          ... bench:     216,145 ns/iter (+/- 3,176)
test bench_nfd_ascii                         ... bench:         361 ns/iter (+/- 5)
test bench_nfd_long                          ... bench:     141,053 ns/iter (+/- 552)
test bench_nfkc_ascii                        ... bench:         587 ns/iter (+/- 8)
test bench_nfkc_long                         ... bench:     225,678 ns/iter (+/- 2,640)
test bench_nfkd_ascii                        ... bench:         333 ns/iter (+/- 7)
test bench_nfkd_long                         ... bench:     149,023 ns/iter (+/- 613)
test bench_streamsafe_adversarial            ... bench:         485 ns/iter (+/- 2)
test bench_streamsafe_ascii                  ... bench:          68 ns/iter (+/- 0)

As you can see, e.g. bench_is_nfd_stream_safe_normalized and bench_nfkc_long have improved while bench_nfkd_long and bench_nfc_ascii have regressed, but none of the changes are dramatic.

cargo test passes, I haven't run any parametric or fuzz testing (not sure if this crate has any). TinyVec itself has been fuzz-tested for equivalence to std::Vec using arbitrary-model-tests framework.

@Manishearth
Copy link
Member

Manishearth commented Mar 16, 2020

I'm usually not super worried about unsafe, however given that we're able to make this change without affecting anything else, I don't mind.

@Manishearth Manishearth merged commit 0d09a94 into unicode-rs:master Mar 16, 2020
@Shnatsel Shnatsel mentioned this pull request Mar 16, 2020
Closed
@Shnatsel Shnatsel deleted the tinyvec branch March 16, 2020 18:05
@8573
Copy link

8573 commented Mar 20, 2020

I noticed that my CI builds for https://github.com/8573/irc-bot.rs are failing on trying to build, as a transitive dependency, smallvec 1.2, whose minimum supported Rust version (MSRV), at 1.36, is higher than mine. cargo tree traces the smallvec 1.2 dependency to unicode-normalization. I see that this crate's MSRV rose to 1.36 because of smallvec (commit cd63ded). With smallvec gone from this crate's dependencies, will this crate's MSRV go back down, or has this crate otherwise come to depend on newer Rust features since commit cd63ded?

@8573
Copy link

8573 commented Mar 20, 2020

Oh, my apologies! I should have checked tinyvec's MSRV — also 1.36.

@CuriouslyCurious CuriouslyCurious mentioned this pull request Mar 23, 2020
Open
@Shnatsel
Copy link
Contributor Author

Shnatsel commented Jun 16, 2020

Could you please make a new release on crates.io incorporating the changes? I see the version has already been bumped in master, and it's been months since the merge.

@Manishearth
Copy link
Member

Manishearth commented Jun 16, 2020

Done

@Shnatsel
Copy link
Contributor Author

Shnatsel commented Jun 16, 2020

Thanks a lot!

@8573 8573 mentioned this pull request Jul 5, 2020
Open
@Manishearth Manishearth mentioned this pull request Jun 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Shnatsel @Manishearth @8573