gh-uuid-rs/uuid
Watch
1
0
Fork
You've already forked uuid
 0
Code Issues 10 Pull requests 2 Releases Activity

Add encode_{lower,upper} to formatting adapters for core::fmt-less rendering. #311

Merged
huonw merged 3 commits from encode into master 2018-09-07 16:09:23 -05:00
Conversation 19 Commits 3 Files changed 7 +836 -60
huonw commented 2018-09-04 22:18:33 -05:00 (Migrated from github.com)

I'm submitting a(n) feature

Description

Add encode_{lower,upper} to formatting adapters for core::fmt-less rendering.

Motivation

These are noticably faster (20ns, ~50%) as they avoid all the virtual
call overhead of core::fmt/write! (etc.). This also improves the
performance of (human-readable) serde encoding, since that previously
went through core::fmt.

Tests

Benchmark results both with LTO and without including a comparison
against crates.io/crates/uuidtoa:

test lto_bench_encode_hyphen ... bench:          16 ns/iter (+/- 1)
test lto_bench_encode_simple ... bench:          14 ns/iter (+/- 1)
test lto_bench_encode_urn    ... bench:          18 ns/iter (+/- 2)
test lto_bench_hyphen        ... bench:          36 ns/iter (+/- 1)
test lto_bench_simple        ... bench:          33 ns/iter (+/- 7)
test lto_bench_urn           ... bench:          40 ns/iter (+/- 4)
test lto_bench_uuidtoa       ... bench:          13 ns/iter (+/- 3)

test nolto_bench_encode_hyphen ... bench:          18 ns/iter (+/- 3)
test nolto_bench_encode_simple ... bench:          19 ns/iter (+/- 3)
test nolto_bench_encode_urn    ... bench:          21 ns/iter (+/- 2)
test nolto_bench_hyphen        ... bench:          41 ns/iter (+/- 11)
test nolto_bench_simple        ... bench:          39 ns/iter (+/- 3)
test nolto_bench_urn           ... bench:          48 ns/iter (+/- 6)
test nolto_bench_uuidtoa       ... bench:          41 ns/iter (+/- 5)

This is now essentially as fast as uuidtoa at its fastest (and
significantly faster than it, without LTO), and the remaining
difference is likely the use of the checked str::from_utf8_mut.

Benchcmp for the existing benchmarks using the existing APIs (only the
ones that changed):

 name                        before.txt ns/iter  after.txt ns/iter  diff ns/iter   diff %  speedup
 lto_bench_hyphen            43                  36                           -7  -16.28%   x 1.19
 lto_bench_json_encode       59 (644 MB/s)       35 (1085 MB/s)              -24  -40.68%   x 1.69
 lto_bench_urn               45                  40                           -5  -11.11%   x 1.12
 nolto_bench_hyphen          48                  41                           -7  -14.58%   x 1.17
 nolto_bench_json_encode     73 (520 MB/s)       45 (844 MB/s)               -28  -38.36%   x 1.62
 nolto_bench_urn             59                  48                          -11  -18.64%   x 1.23

That is, JSON encoding takes 40% less time, and even core::fmt
formatting for the formats that use hyphens got slightly faster too.

Related Issue(s)

Closes #196.

<!-- If this PR is a breaking change, ensure that you are opening it against the `breaking` branch. If the pull request is incomplete, prepend the Title with WIP: --> **I'm submitting a(n)** feature # Description Add encode_{lower,upper} to formatting adapters for core::fmt-less rendering. # Motivation These are noticably faster (20ns, ~50%) as they avoid all the virtual call overhead of core::fmt/write! (etc.). This also improves the performance of (human-readable) serde encoding, since that previously went through core::fmt. # Tests Benchmark results both with LTO and without including a comparison against crates.io/crates/uuidtoa: ``` test lto_bench_encode_hyphen ... bench: 16 ns/iter (+/- 1) test lto_bench_encode_simple ... bench: 14 ns/iter (+/- 1) test lto_bench_encode_urn ... bench: 18 ns/iter (+/- 2) test lto_bench_hyphen ... bench: 36 ns/iter (+/- 1) test lto_bench_simple ... bench: 33 ns/iter (+/- 7) test lto_bench_urn ... bench: 40 ns/iter (+/- 4) test lto_bench_uuidtoa ... bench: 13 ns/iter (+/- 3) test nolto_bench_encode_hyphen ... bench: 18 ns/iter (+/- 3) test nolto_bench_encode_simple ... bench: 19 ns/iter (+/- 3) test nolto_bench_encode_urn ... bench: 21 ns/iter (+/- 2) test nolto_bench_hyphen ... bench: 41 ns/iter (+/- 11) test nolto_bench_simple ... bench: 39 ns/iter (+/- 3) test nolto_bench_urn ... bench: 48 ns/iter (+/- 6) test nolto_bench_uuidtoa ... bench: 41 ns/iter (+/- 5) ``` This is now essentially as fast as uuidtoa at its fastest (and significantly faster than it, without LTO), and the remaining difference is likely the use of the checked `str::from_utf8_mut`. Benchcmp for the existing benchmarks using the existing APIs (only the ones that changed): ``` name before.txt ns/iter after.txt ns/iter diff ns/iter diff % speedup lto_bench_hyphen 43 36 -7 -16.28% x 1.19 lto_bench_json_encode 59 (644 MB/s) 35 (1085 MB/s) -24 -40.68% x 1.69 lto_bench_urn 45 40 -5 -11.11% x 1.12 nolto_bench_hyphen 48 41 -7 -14.58% x 1.17 nolto_bench_json_encode 73 (520 MB/s) 45 (844 MB/s) -28 -38.36% x 1.62 nolto_bench_urn 59 48 -11 -18.64% x 1.23 ``` That is, JSON encoding takes 40% less time, and even core::fmt formatting for the formats that use hyphens got slightly faster too. # Related Issue(s) Closes #196.
radix (Migrated from github.com) reviewed 2018-09-04 22:18:33 -05:00
Dylan-DPC-zz (Migrated from github.com) reviewed 2018-09-04 22:18:33 -05:00
todo[bot] commented 2018-09-04 22:18:38 -05:00 (Migrated from github.com)

Self doesn't work https://github.com/rust-lang/rust/issues/52808

github.com/uuid-rs/uuid@cf6703d599/src/adapter/core_support/mod.rs (L28-L33)

This comment was generated by todo based on a TODO comment in cf6703d5998d602d85c532f6d378625cd5275956 in #311. cc @huonw.
## Self doesn&#x27;t work https://github.com/rust-lang/rust/issues/52808 https://github.com/uuid-rs/uuid/blob/cf6703d5998d602d85c532f6d378625cd5275956/src/adapter/core_support/mod.rs#L28-L33 --- ###### This comment was generated by [todo](https://todo.jasonet.co) based on a `TODO` comment in cf6703d5998d602d85c532f6d378625cd5275956 in #311. cc @huonw.
todo[bot] commented 2018-09-04 22:40:50 -05:00 (Migrated from github.com)

Self doesn't work https://github.com/rust-lang/rust/issues/52808

github.com/uuid-rs/uuid@dcdbe7b19e/src/adapter/core_support/mod.rs (L28-L33)

This comment was generated by todo based on a TODO comment in dcdbe7b19e4dd0e38fbb7f08b319a5db412531dc in #311. cc @huonw.
## Self doesn&#x27;t work https://github.com/rust-lang/rust/issues/52808 https://github.com/uuid-rs/uuid/blob/dcdbe7b19e4dd0e38fbb7f08b319a5db412531dc/src/adapter/core_support/mod.rs#L28-L33 --- ###### This comment was generated by [todo](https://todo.jasonet.co) based on a `TODO` comment in dcdbe7b19e4dd0e38fbb7f08b319a5db412531dc in #311. cc @huonw.
KodrAus (Migrated from github.com) reviewed 2018-09-05 01:09:18 -05:00
KodrAus (Migrated from github.com) left a comment

Thanks @huonw!

The additional docs, tests and benches are great. I've just left a few little nitpicks.

Thanks @huonw! The additional docs, tests and benches are great. I've just left a few little nitpicks.
src/adapter/mod.rs
@ -187,6 +188,67 @@ impl Uuid {
}
KodrAus (Migrated from github.com) commented 2018-09-05 01:02:59 -05:00

It might be nicer to use expect with a message here indicating that this shouldn't happen rather than unwrap.

It might be nicer to use `expect` with a message here indicating that this shouldn't happen rather than `unwrap`.
KodrAus (Migrated from github.com) commented 2018-09-05 01:04:35 -05:00

Is there a reason to pass a start index in rather than expect full_buffer to have been sliced?

Is there a reason to pass a start index in rather than expect `full_buffer` to have been sliced?
huonw (Migrated from github.com) reviewed 2018-09-05 10:31:48 -05:00
src/adapter/mod.rs
@ -187,6 +188,67 @@ impl Uuid {
}
huonw (Migrated from github.com) commented 2018-09-05 10:31:48 -05:00

Yeah, for the URN case: there's a prefix that gets added in front of what encode does, but I want to have that prefix included in the return value, and thus also in the from_utf8_mut call in this function.

Yeah, for the URN case: there's a prefix that gets added in front of what `encode` does, but I want to have that prefix included in the return value, and thus also in the `from_utf8_mut` call in this function.
KodrAus (Migrated from github.com) reviewed 2018-09-05 16:05:14 -05:00
src/adapter/mod.rs
@ -187,6 +188,67 @@ impl Uuid {
}
KodrAus (Migrated from github.com) commented 2018-09-05 16:05:14 -05:00

Ah gotcha 👍

Ah gotcha :+1:
kinggoesgaming (Migrated from github.com) reviewed 2018-09-05 22:17:24 -05:00
src/serde_support.rs
@ -20,3 +20,3 @@
) -> Result<S::Ok, S::Error> {
if serializer.is_human_readable() {
serializer.collect_str(&self.to_hyphenated())
serializer
kinggoesgaming (Migrated from github.com) commented 2018-09-05 22:17:24 -05:00

just a question: was collect_str the wrong function to use? curious..

just a question: was `collect_str` the wrong function to use? curious..
huonw (Migrated from github.com) reviewed 2018-09-05 23:47:13 -05:00
src/serde_support.rs
@ -20,3 +20,3 @@
) -> Result<S::Ok, S::Error> {
if serializer.is_human_readable() {
serializer.collect_str(&self.to_hyphenated())
serializer
huonw (Migrated from github.com) commented 2018-09-05 23:47:13 -05:00

I don't think so, it's much more direct and less error prone than manually using the Display impls to construct a str. This new APIs is only a little bit more complicated to use with serialize_str than collect_str, so I think it makes sense to switch.

I don't think so, it's much more direct and less error prone than manually using the `Display` impls to construct a `str`. This new APIs is only a little bit more complicated to use with `serialize_str` than `collect_str`, so I think it makes sense to switch.
todo[bot] commented 2018-09-06 23:48:05 -05:00 (Migrated from github.com)

Self doesn't work https://github.com/rust-lang/rust/issues/52808

github.com/uuid-rs/uuid@4eee4f18ce/src/adapter/core_support/mod.rs (L28-L33)

This comment was generated by todo based on a TODO comment in 4eee4f18ce57d902c8882e0eb47b1494552d1d38 in #311. cc @huonw.
## Self doesn&#x27;t work https://github.com/rust-lang/rust/issues/52808 https://github.com/uuid-rs/uuid/blob/4eee4f18ce57d902c8882e0eb47b1494552d1d38/src/adapter/core_support/mod.rs#L28-L33 --- ###### This comment was generated by [todo](https://todo.jasonet.co) based on a `TODO` comment in 4eee4f18ce57d902c8882e0eb47b1494552d1d38 in #311. cc @huonw.
todo[bot] commented 2018-09-06 23:48:22 -05:00 (Migrated from github.com)

Self doesn't work https://github.com/rust-lang/rust/issues/52808

github.com/uuid-rs/uuid@246c7f7586/src/adapter/core_support/mod.rs (L28-L33)

This comment was generated by todo based on a TODO comment in 246c7f7586 in #311. cc @huonw.
## Self doesn&#x27;t work https://github.com/rust-lang/rust/issues/52808 https://github.com/uuid-rs/uuid/blob/246c7f758686d574ac59873a3687b2936880f867/src/adapter/core_support/mod.rs#L28-L33 --- ###### This comment was generated by [todo](https://todo.jasonet.co) based on a `TODO` comment in 246c7f758686d574ac59873a3687b2936880f867 in #311. cc @huonw.
huonw (Migrated from github.com) reviewed 2018-09-06 23:48:33 -05:00
src/adapter/mod.rs
@ -187,6 +188,67 @@ impl Uuid {
}
huonw (Migrated from github.com) commented 2018-09-06 23:48:33 -05:00

Done.

Done.
huonw (Migrated from github.com) reviewed 2018-09-06 23:52:21 -05:00
src/adapter/mod.rs
@ -187,6 +188,67 @@ impl Uuid {
}
huonw (Migrated from github.com) commented 2018-09-06 23:52:21 -05:00

I added a doc comment to encode that explains this.

I added a doc comment to `encode` that explains this.
KodrAus (Migrated from github.com) approved these changes 2018-09-07 00:50:38 -05:00
KodrAus (Migrated from github.com) left a comment

This looks good to me! Thanks @huonw

This looks good to me! Thanks @huonw
KodrAus commented 2018-09-07 00:51:59 -05:00 (Migrated from github.com)

bors r+

bors r+
kinggoesgaming commented 2018-09-07 10:26:02 -05:00 (Migrated from github.com)

@KodrAus we are waiting until 0.7.0 release before merging this

@KodrAus we are waiting until `0.7.0` release before merging this
todo[bot] commented 2018-09-07 13:09:57 -05:00 (Migrated from github.com)

Self doesn't work https://github.com/rust-lang/rust/issues/52808

github.com/uuid-rs/uuid@469acc7634/src/adapter/core_support/mod.rs (L28-L33)

This comment was generated by todo based on a TODO comment in 469acc7634 in #311. cc @huonw.
## Self doesn&#x27;t work https://github.com/rust-lang/rust/issues/52808 https://github.com/uuid-rs/uuid/blob/469acc7634533ba01c756d08b4d45f24d2bc3e6b/src/adapter/core_support/mod.rs#L28-L33 --- ###### This comment was generated by [todo](https://todo.jasonet.co) based on a `TODO` comment in 469acc7634533ba01c756d08b4d45f24d2bc3e6b in #311. cc @huonw.
Dylan-DPC-zz commented 2018-09-07 16:00:00 -05:00 (Migrated from github.com)

bors: r+

bors: r+
bors[bot] commented 2018-09-07 16:09:22 -05:00 (Migrated from github.com)

Build succeeded

# Build succeeded * [continuous-integration/appveyor/branch](https://ci.appveyor.com/project/KodrAus/uuid/build/1.0.940) * [continuous-integration/travis-ci/push](https://travis-ci.org/uuid-rs/uuid/builds/425914448?utm_source=github_status&utm_medium=notification)
Sign in to join this conversation.
Reviewers
No reviewers
radix
Dylan-DPC-zz
KodrAus
Labels
Clear labels   
breaking
This causes a breaking change and will be merged to breaking branch.

  
bug

   
dependencies
Pull requests that update a dependency file

  
good first issue

   
help wanted

   
unstable
Tracking issue for an unstable optional dependency

No labels
breaking
bug
dependencies
good first issue
help wanted
unstable
Milestone
Clear milestone
No items
No milestone
0.7.2
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-uuid-rs/uuid!311
No description provided.