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.

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

Remove cast.rs use try_into when as_* is used #1

Open
RCasatta opened this issue Oct 10, 2020 · 3 comments
Open

Remove cast.rs use try_into when as_* is used #1

RCasatta opened this issue Oct 10, 2020 · 3 comments

Comments

@RCasatta
Copy link
Owner

No description provided.

anforowicz added a commit to anforowicz/qr_code that referenced this issue Apr 20, 2023
@anforowicz
Use TryInto in all configurations.
e022d2f 
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
@anforowicz
Copy link
Contributor

FWIW, the performance impact of always using TryInto is fairly small - see e022d2f for more details.

OTOH, it would be great if fallible conversions could be avoided altogether. Unfortunately this is a bit challenging because of the public API:

  • Canvas::get takes coordinates as i16 and needs to convert it to usize. If the parameter was u16 then conversion to usize would be infallible and could be done through the Into trait.
  • Version stores the numeric version value as i16. Even though this is an internal API, the choice of the type impacts some public APIs like Version::width which also returns i16 (and would require conversions if the storage was in u16 or another unsigned integer type).

@RCasatta
Copy link
Owner Author

Given your Evaluation section in e022d2f I would accept the tradeoff of losing a bit of perfomance/size in exchange for correctness/simplification.

I think it's also possible to consider a breaking change in the public API in the future for a 2.x version

@RCasatta
Copy link
Owner Author

I didn't evaluate all implications, but I would also consider addressing clippy::wrong_self_convention with an MR working on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@RCasatta @anforowicz