Skip to content

Commit

Permalink
Use TryInto in all configurations.
Browse files Browse the repository at this point in the history
Summary
-------

This commit is a step toward removing `cast.rs` (see
RCasatta#1).  This commit tweaks
`cast.rs` to uniformly use `TryInto` in all configurations (not just in
`#[cfg(debug_assertions)]`).

Benchmark impact
----------------

Disclaimers: Benchmarking is tricky:

* Confidence intervals below are relatively wide
* Code changes can have indirect/unexpected impact (e.g. noise from
  random changes to binary code layout may be significant in
  microbenchmarks).

This commit introduces additional checks into the relase configuration
and therefore may have a performance impact.  Benchmarks before this
commit:

    $ cargo bench --features=bench | grep bench
    ...
    ...bench_qr_code_with_low_ecc     ...:  18,680,806 ns/iter (+/- 147,143)
    ...bench_find_min_version         ...:           1 ns/iter (+/- 0)
    ...bench_push_splitted_bytes      ...:       3,754 ns/iter (+/- 48)
    ...bench::bench_optimize_base64   ...:      29,262 ns/iter (+/- 470)
    ...bench::bench_optimize_example1 ...:       1,582 ns/iter (+/- 33)

Benchmarks after this commit:

    $ cargo bench --features=bench | grep bench
    ...
    ...bench_qr_code_with_low_ecc     ...:  18,805,800 ns/iter (+/- 133,211)
    ...bench_find_min_version         ...:           1 ns/iter (+/- 0)
    ...bench_push_splitted_bytes      ...:       4,070 ns/iter (+/- 19)
    ...bench::bench_optimize_base64   ...:      36,202 ns/iter (+/- 1,069)
    ...bench::bench_optimize_example1 ...:       1,706 ns/iter (+/- 23)

The difference is 18,805,800 ns - 18,680,806 ns = 124,994 ns.  This
is a slowdown of 124,994 ns / 18,805,800 ns = 0.664%.

Evaluation
----------

Arguments for landing this commit:

* Potential for code removal and simplification
* Consistently applying correctness checks in all configurations.
    * This is consistent with Rust's safe-by-default approach
    * This is consistent with the recent change in Chromium policy which
      recommends against using debug-mode-only `DCHECK`s:
      https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md

Arguments against landing this commit:

* 0.664% slowdown (note the disclaimers above about micro-benchmarks)
* Extra checks may increase the binary size of the code
  • Loading branch information
anforowicz committed Apr 20, 2023
1 parent b92f099 commit e022d2f
Showing 1 changed file with 0 additions and 26 deletions.
26 changes: 0 additions & 26 deletions src/cast.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
#[cfg(debug_assertions)]
use std::convert::TryInto;

use std::fmt::Display;

// TODO remove this, use try_into wher as_* is used
Expand Down Expand Up @@ -41,47 +39,23 @@ impl<T> ExpectOrOverflow for Option<T> {

macro_rules! impl_as {
($ty:ty) => {
#[cfg(debug_assertions)]
impl As for $ty {
fn as_u16(self) -> u16 {
self.try_into().unwrap()
}

fn as_i16(self) -> i16 {
self.try_into().unwrap()
}

fn as_u32(self) -> u32 {
self.try_into().unwrap()
}

fn as_usize(self) -> usize {
self.try_into().unwrap()
}

fn as_isize(self) -> isize {
self.try_into().unwrap()
}
}

#[cfg(not(debug_assertions))]
impl As for $ty {
fn as_u16(self) -> u16 {
self as u16
}
fn as_i16(self) -> i16 {
self as i16
}
fn as_u32(self) -> u32 {
self as u32
}
fn as_usize(self) -> usize {
self as usize
}
fn as_isize(self) -> isize {
self as isize
}
}
};
}

Expand Down

0 comments on commit e022d2f

Please sign in to comment.