Replace call by reference by call by value #58
Conversation
This reverts commit 42eef93.
| } | ||
| } | ||
| } | ||
|
|
||
| impl Add<ProjectiveNielsPoint> for ExtendedPoint { |
There was a problem hiding this comment.
Should this impl be using one of the macros you defined?
| add_impl!(FieldElement, FieldElement, FieldElement); | ||
| sub_impl!(FieldElement, FieldElement, FieldElement); | ||
| mul_impl!(FieldElement, FieldElement, FieldElement); | ||
| neg_impl!(FieldElement, FieldElement); |
There was a problem hiding this comment.
This syntax is a bit confusing -- maybe it would be better if the macro invocation looked like
mul_impl!( LHS = FieldElement, RHS = FieldElement, Output = FieldElement )
neg_impl!( Self = FieldElement, Output = FieldElement )
or something similar, so that the roles of the parameters were clear just by looking?
There was a problem hiding this comment.
This looks good, the only comments I had were cosmetic. I benchmarked against master and there's a slight performance increase (2-6%) on most of the benchmarks, which is really nice!
I think it would be good if the macro-generated operator implementations also defined mixed operators, but we could add that later if you want.
| // Various macros for implmenting repetative pass-by-value semantics | ||
|
|
||
| #[macro_export] | ||
| macro_rules! add_impl { |
There was a problem hiding this comment.
Maybe we should extend these macros to also generate implementations of "mixed" operators like &FieldElement * FieldElement and FieldElement * &FieldElement, so that all cases of borrow / non-borrow are covered?
There was a problem hiding this comment.
Yeah, I was considering adding those later if this PR goes through.
|
@hdevalence On the issue (#57) I posted my benchmarks. That seems in the range of the difference I saw. Keep in mind that's including removing most of the Eye of Saurons from the library code too. I wouldn't be surprised if the performance penalty is simply because I messed up and forgot to add inline tags everywhere I was supposed to. |
src/curve.rs
Outdated
| @@ -887,7 +889,7 @@ impl<'a> Neg for &'a AffineNielsPoint { | |||
| // ------------------------------------------------------------------------ | |||
|
|
|||
| impl<'b> MulAssign<&'b Scalar> for ExtendedPoint { | |||
| #[inline] | |||
| #[inline(always)] | |||
| fn mul_assign(&mut self, scalar: &'b Scalar) { | |||
| let result = (self as &ExtendedPoint) * scalar; | |||
There was a problem hiding this comment.
Wouldn't &*self be simpler here?
|
Closing due to many conflicts and #101 fixing this for the most part. |
Also does a pass through the docs converting `TypeNames` to Rustdoc links, and making sure that all the items have consistent summaries. Closes dalek-cryptography#58 Closes $56
Tracking issue: #57
Implements
Add,SubandMulon value variants ofFieldElement,ExtendedElementandScalar.At the same time gets rid of "Eye of Sauron" (arithmetic that looks like
&(&x + &y) * &z.square()) expressions. Difference in performance can be compared to master and seems to be in the low ns for operatations onFieldElements however is higher on more complex operations.This PR isn't ready to be merged until the performance issues are resolved.