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

huonw · 2018-09-05T03:18:33Z

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.

Current results: test bincode_decode ... bench: 14 ns/iter (+/- 2) = 1714 MB/s test bincode_encode ... bench: 11 ns/iter (+/- 2) = 2181 MB/s test json_decode ... bench: 98 ns/iter (+/- 6) = 387 MB/s test json_encode ... bench: 71 ns/iter (+/- 7) = 535 MB/s

todo · 2018-09-05T03:18:38Z

Self doesn't work rust-lang/rust#52808

uuid/src/adapter/core_support/mod.rs

Lines 28 to 33 in cf6703d

    
           impl fmt::Display for super::Simple { 
        
               #[inline] 
        
               fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { 
        
                   fmt::LowerHex::fmt(self, f) 
        
               }

This comment was generated by todo based on a `TODO` comment in `cf6703d` in #311. cc @huonw.

todo · 2018-09-05T03:40:50Z

Self doesn't work rust-lang/rust#52808

uuid/src/adapter/core_support/mod.rs

Lines 28 to 33 in dcdbe7b

    
           impl fmt::Display for super::Simple { 
        
               #[inline] 
        
               fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { 
        
                   fmt::LowerHex::fmt(self, f) 
        
               }

This comment was generated by todo based on a `TODO` comment in `dcdbe7b` in #311. cc @huonw.

KodrAus

Thanks @huonw!

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

KodrAus · 2018-09-05T06:02:59Z

src/adapter/mod.rs

+        }
+    }
+
+    str::from_utf8_mut(&mut full_buffer[..start + len]).unwrap()


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

KodrAus · 2018-09-05T06:04:35Z

src/adapter/mod.rs

+
+fn encode<'a>(
+    full_buffer: &'a mut [u8],
+    start: usize,


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

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.

Ah gotcha 👍

I added a doc comment to encode that explains this.

src/serde_support.rs

todo · 2018-09-07T04:48:05Z

Self doesn't work rust-lang/rust#52808

uuid/src/adapter/core_support/mod.rs

Lines 28 to 33 in 4eee4f1

    
           impl fmt::Display for super::Simple { 
        
               #[inline] 
        
               fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { 
        
                   fmt::LowerHex::fmt(self, f) 
        
               }

This comment was generated by todo based on a `TODO` comment in `4eee4f1` in #311. cc @huonw.

…ndering. 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. 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. Closes uuid-rs#196.

todo · 2018-09-07T04:48:22Z

Self doesn't work rust-lang/rust#52808

uuid/src/adapter/core_support/mod.rs

Lines 28 to 33 in 246c7f7

    
           impl fmt::Display for super::Simple { 
        
               #[inline] 
        
               fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { 
        
                   fmt::LowerHex::fmt(self, f) 
        
               }

This comment was generated by todo based on a `TODO` comment in `246c7f7` in #311. cc @huonw.

KodrAus

This looks good to me! Thanks @huonw

KodrAus · 2018-09-07T05:51:59Z

bors r+

311: Add encode_{lower,upper} to formatting adapters for core::fmt-less rendering. r=KodrAus a=huonw  **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. Co-authored-by: Huon Wilson <huon.wilson@data61.csiro.au>

kinggoesgaming · 2018-09-07T15:26:02Z

@KodrAus we are waiting until 0.7.0 release before merging this

todo · 2018-09-07T18:09:57Z

Self doesn't work rust-lang/rust#52808

uuid/src/adapter/core_support/mod.rs

Lines 28 to 33 in 469acc7

    
           impl fmt::Display for super::Simple { 
        
               #[inline] 
        
               fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { 
        
                   fmt::LowerHex::fmt(self, f) 
        
               }

This comment was generated by todo based on a `TODO` comment in `469acc7` in #311. cc @huonw.

Dylan-DPC-zz · 2018-09-07T21:00:00Z

bors: r+

311: Add encode_{lower,upper} to formatting adapters for core::fmt-less rendering. r=Dylan-DPC a=huonw  **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. Co-authored-by: Huon Wilson <huon.wilson@data61.csiro.au> Co-authored-by: Hunar Roop Kahlon <hunar.roop@gmail.com>

bors · 2018-09-07T21:09:22Z

Build succeeded

317: make encode_ functions pub(crate) for now r=kinggoesgaming a=kinggoesgaming **I'm submitting a(n)** other # Description Includes: * `uuid::Uuid::encode_buffer() -> [u8; adapter::Urn::LENGTH]` * <adapter>::{encode_lower, encode_upper} # Motivation The function signature _may_ change but we don't want a breaking change after `0.7.1` if we can help it. # Tests Tests pass :) # Related Issue(s) #311 Co-authored-by: Hunar Roop Kahlon <hunar.roop@gmail.com>

huonw requested review from Dylan-DPC-zz, kinggoesgaming, KodrAus and radix as code owners September 5, 2018 03:18

KodrAus reviewed Sep 5, 2018

View reviewed changes

kinggoesgaming added this to the 0.7.1 milestone Sep 5, 2018

kinggoesgaming added this to In progress in 0.7.x via automation Sep 5, 2018

kinggoesgaming added this to Needs triage in Pull Request Triage via automation Sep 5, 2018

kinggoesgaming added the Type:Enhancement label Sep 5, 2018

kinggoesgaming moved this from Needs triage to Low priority in Pull Request Triage Sep 5, 2018

kinggoesgaming reviewed Sep 6, 2018

View reviewed changes

src/serde_support.rs Show resolved Hide resolved

huonw force-pushed the encode branch from dcdbe7b to 4eee4f1 Compare September 7, 2018 04:47

huonw force-pushed the encode branch from 4eee4f1 to 246c7f7 Compare September 7, 2018 04:48

0.7.x automation moved this from In progress to Reviewer approved Sep 7, 2018

KodrAus approved these changes Sep 7, 2018

View reviewed changes

Merge branch 'master' into encode

469acc7

bors bot merged commit 469acc7 into uuid-rs:master Sep 7, 2018

0.7.x automation moved this from Reviewer approved to Done Sep 7, 2018

Pull Request Triage automation moved this from Low priority to Closed Sep 7, 2018

kinggoesgaming mentioned this pull request Sep 13, 2018

make encode_ functions pub(crate) for now #317

Merged

kinggoesgaming modified the milestones: 0.7.1, 0.7.2 Sep 13, 2018

huonw deleted the encode branch December 6, 2018 02:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

huonw commented Sep 5, 2018

todo bot commented Sep 5, 2018

todo bot commented Sep 5, 2018

KodrAus left a comment

KodrAus Sep 5, 2018

huonw Sep 7, 2018

KodrAus Sep 5, 2018

huonw Sep 5, 2018

KodrAus Sep 5, 2018

huonw Sep 7, 2018

todo bot commented Sep 7, 2018

todo bot commented Sep 7, 2018

KodrAus left a comment

KodrAus commented Sep 7, 2018

kinggoesgaming commented Sep 7, 2018 •

edited

todo bot commented Sep 7, 2018

Dylan-DPC-zz commented Sep 7, 2018

bors bot commented Sep 7, 2018

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

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

Conversation

huonw commented Sep 5, 2018

Description

Motivation

Tests

Related Issue(s)

todo bot commented Sep 5, 2018

Self doesn't work rust-lang/rust#52808

This comment was generated by todo based on a TODO comment in cf6703d in #311. cc @huonw.

todo bot commented Sep 5, 2018

Self doesn't work rust-lang/rust#52808

This comment was generated by todo based on a TODO comment in dcdbe7b in #311. cc @huonw.

KodrAus left a comment

Choose a reason for hiding this comment

KodrAus Sep 5, 2018

Choose a reason for hiding this comment

huonw Sep 7, 2018

Choose a reason for hiding this comment

KodrAus Sep 5, 2018

Choose a reason for hiding this comment

huonw Sep 5, 2018

Choose a reason for hiding this comment

KodrAus Sep 5, 2018

Choose a reason for hiding this comment

huonw Sep 7, 2018

Choose a reason for hiding this comment

todo bot commented Sep 7, 2018

Self doesn't work rust-lang/rust#52808

This comment was generated by todo based on a TODO comment in 4eee4f1 in #311. cc @huonw.

todo bot commented Sep 7, 2018

Self doesn't work rust-lang/rust#52808

This comment was generated by todo based on a TODO comment in 246c7f7 in #311. cc @huonw.

KodrAus left a comment

Choose a reason for hiding this comment

KodrAus commented Sep 7, 2018

kinggoesgaming commented Sep 7, 2018 • edited

todo bot commented Sep 7, 2018

Self doesn't work rust-lang/rust#52808

This comment was generated by todo based on a TODO comment in 469acc7 in #311. cc @huonw.

Dylan-DPC-zz commented Sep 7, 2018

bors bot commented Sep 7, 2018

Build succeeded

This comment was generated by todo based on a `TODO` comment in `cf6703d` in #311. cc @huonw.

This comment was generated by todo based on a `TODO` comment in `dcdbe7b` in #311. cc @huonw.

This comment was generated by todo based on a `TODO` comment in `4eee4f1` in #311. cc @huonw.

This comment was generated by todo based on a `TODO` comment in `246c7f7` in #311. cc @huonw.

kinggoesgaming commented Sep 7, 2018 •

edited

This comment was generated by todo based on a `TODO` comment in `469acc7` in #311. cc @huonw.