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.

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

k256: impl ff and group traits #164

Merged
merged 4 commits into from
Sep 5, 2020
Merged

k256: impl ff and group traits #164

merged 4 commits into from
Sep 5, 2020

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Sep 4, 2020

  • Impls ff::{Field, PrimeField} on Scalar
  • Impls group::{Group, Curve} on ProjectivePoint

@tarcieri tarcieri requested review from fjarri, tuxxy and str4d September 4, 2020 18:02
@tarcieri
Copy link
Member Author

tarcieri commented Sep 4, 2020

Lots of clippy complaints about unnecessary borrows now. I'm wondering if we need to add #[inline] on the various owned arithmetic operations to remove those without impacting performance. Edit: applied clippy fixes and posted benchmarks below.

It also seems like there are a lot of inherent methods on Scalar and ProjectivePoint which also have similar or identical methods on the trait. Perhaps we should get rid of the duplicated inherent methods and just use the trait?

@tarcieri
Copy link
Member Author

tarcieri commented Sep 4, 2020

Applied clippy's suggested fixes with the new arithmetic impls on owned types. I do really like how it improves the code quality (by removing "eye of sauron"-style borrowed arithmetic).

Ran the existing benchmarks... it looked like mostly improvements except for normalize:

Screen Shot 2020-09-04 at 11 56 37 AM

field element operations/normalize_weak
                        time:   [1.6940 ns 1.7020 ns 1.7113 ns]
                        change: [+4.7960% +5.6804% +6.5209%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) low mild
  5 (5.00%) high mild
  1 (1.00%) high severe
field element operations/normalize
                        time:   [1.7010 ns 1.7075 ns 1.7142 ns]
                        change: [+1.4923% +3.0551% +4.6100%] (p = 0.00 < 0.05)
                        Performance has regressed.

cc @fjarri

@fjarri
Copy link
Contributor

fjarri commented Sep 4, 2020

I wouldn't worry about it. The change is small, and when we're talking about an operation that takes literally 1ns, I doubt that these measurements are very accurate. On my laptop it takes ~4ns, and I don't see any change between this PR and master.

@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2020

Codecov Report

Merging #164 into master will decrease coverage by 1.21%.
The diff coverage is 24.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
- Coverage   60.64%   59.43%   -1.22%     
==========================================
  Files          25       25              
  Lines        3601     3688      +87     
==========================================
+ Hits         2184     2192       +8     
- Misses       1417     1496      +79     
Impacted Files Coverage Δ
k256/src/arithmetic/affine.rs 85.88% <0.00%> (-2.07%) ⬇️
k256/src/arithmetic/scalar/scalar_4x64.rs 0.00% <0.00%> (ø)
k256/src/arithmetic/scalar/scalar_8x32.rs 91.66% <0.00%> (-0.80%) ⬇️
k256/src/arithmetic/scalar.rs 76.60% <12.76%> (-13.86%) ⬇️
k256/src/arithmetic/projective.rs 76.73% <18.75%> (-8.85%) ⬇️
k256/src/arithmetic/mul.rs 89.06% <100.00%> (-2.88%) ⬇️
k256/src/ecdsa/recoverable.rs 62.33% <100.00%> (ø)
k256/src/ecdsa/sign.rs 29.82% <100.00%> (ø)
k256/src/ecdsa/verify.rs 51.42% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d81c5c0...4fe63b0. Read the comment docs.

- Impls `ff::{Field, PrimeField}` on `Scalar`
- Impls `group::{Group, Curve}` on `ProjectivePoint`
@tarcieri tarcieri changed the title [WIP] k256: impl ff and group traits k256: impl ff and group traits Sep 5, 2020
@tarcieri tarcieri marked this pull request as ready for review September 5, 2020 19:11
@tarcieri tarcieri merged commit ebac050 into master Sep 5, 2020
@tarcieri tarcieri deleted the k256/group-traits branch September 5, 2020 19:20
tarcieri added a commit that referenced this pull request Sep 6, 2020
Corresponding change to #164, but for the `p256` crate.

- Impls `ff::{Field, PrimeField}` on `Scalar`
- Impls `group::{Group, Curve}` on `ProjectivePoint`
tarcieri added a commit that referenced this pull request Sep 6, 2020
Corresponding change to #164, but for the `p256` crate.

- Impls `ff::{Field, PrimeField}` on `Scalar`
- Impls `group::{Group, Curve}` on `ProjectivePoint`
tarcieri added a commit that referenced this pull request Sep 6, 2020
Corresponding change to #164, but for the `p256` crate.

- Impls `ff::{Field, PrimeField}` on `Scalar`
- Impls `group::{Group, Curve}` on `ProjectivePoint`
tarcieri added a commit that referenced this pull request Sep 6, 2020
Corresponding change to #164, but for the `p256` crate.

- Impls `ff::{Field, PrimeField}` on `Scalar`
- Impls `group::{Group, Curve}` on `ProjectivePoint`
@tarcieri tarcieri mentioned this pull request Sep 6, 2020
This was referenced Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants