-
Notifications
You must be signed in to change notification settings - Fork 59
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
Merge old optimisations from spdz2k-develop #385
Conversation
Codecov Report
@@ Coverage Diff @@
## master #385 +/- ##
============================================
Coverage 100.00% 100.00%
- Complexity 3485 3597 +112
============================================
Files 394 404 +10
Lines 9878 10177 +299
Branches 758 776 +18
============================================
+ Hits 9878 10177 +299
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I only have a couple of suggestions for clarification (which is not really an issue with the merge, but rather an issue with the state of spdz2k-develop code base).
return builder.seq(new Equality(bitLength, x, y)); | ||
public DRes<SInt> equals(DRes<SInt> x, DRes<SInt> y) { | ||
int maxBitLength = builder.getBasicNumericContext().getMaxBitLength(); | ||
return equals(x, y, maxBitLength); | ||
} | ||
|
||
@Override | ||
public DRes<SInt> compareLEQ(DRes<SInt> x, DRes<SInt> y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this algorithm is now obsolete. However for compatibility it can instead be implemented by calling compareLT
on x-1 or y+1 (depending on whether there are issues with overflow or underflow)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It now calls compareLT and is annotated as being deprecated, but I keep it around for now since it is used in many places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think maybe we should add a task to fix this? Because then it means that certain protocols use one (less fast) protocol for comparison and others use the efficient version.
@@ -1,4 +1,4 @@ | |||
package dk.alexandra.fresco.lib.common.compare.gt; | |||
package dk.alexandra.fresco.lib.common.compare.lt; | |||
|
|||
import dk.alexandra.fresco.framework.DRes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this call is now obsolete, unless it uses an algorithm that can be better than the constant round and log round comparison algorithms now implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is used in the LP lib to compare large values, but besides that, it is obsolete. I have marked it as deprecated for now.
lib/common/src/main/java/dk/alexandra/fresco/lib/common/math/integer/binary/RightShift.java
Outdated
Show resolved
Hide resolved
lib/common/src/main/java/dk/alexandra/fresco/lib/common/math/integer/binary/Truncate.java
Outdated
Show resolved
Hide resolved
lib/common/src/main/java/dk/alexandra/fresco/lib/common/math/integer/mod/Mod2m.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have made a new issue for removing remaining deprecated code. Otherwise good.
return builder.seq(new Equality(bitLength, x, y)); | ||
public DRes<SInt> equals(DRes<SInt> x, DRes<SInt> y) { | ||
int maxBitLength = builder.getBasicNumericContext().getMaxBitLength(); | ||
return equals(x, y, maxBitLength); | ||
} | ||
|
||
@Override | ||
public DRes<SInt> compareLEQ(DRes<SInt> x, DRes<SInt> y) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think maybe we should add a task to fix this? Because then it means that certain protocols use one (less fast) protocol for comparison and others use the efficient version.
No description provided.