Closed alanefl closed 5 years ago
Ok let's do this review in stages cuz it's quite large. Before we get started I'll say that the class group implementation itself looks to be pretty much good, but that most of the changes to be made are around how to integrate it with our benchmarking etc.
The first thing I'd like you to change is to restructure the code in a way such that the "dev" feature is unnecessary. It's definitely a wart to require it in our workflow.
Here's how:
square
can be publicnew_raw
can be public (I'd rename to raw
to be shorter), and it should eagerly reduce its inputnormalize_pub
and reduce_pub
can be removedSure, we can work around the dev
flag. We're sacrificing modularity for convenience in workflow by doing this, but I don't feel strongly about whether to favor one or the other. If we later wrap lower-level library functionality in a hazardous
module, then I'm even more OK with leaking these functions to be public. Just note that we're making public things that have on other reason to be public besides benchmarking.
(Just a note, we should have a broader conversation about using --features
soon -- for some of the optimizations @mstraka100 and I are working on, we're gonna require some external C libraries, and we're gonna have to think about the tradeoffs between speed and the convenience of packaging/ installing the library)
Cool. I'm wrapping up for the day but it's looking good at a high level. I'll give the line-by-line a look over tomorrow.
You may have checked already but is it possible to also use a macro to reduce test duplication (similar to how you use a macro for benchmark duplication)?
also, don't forget to remove the dev flag from cargo.toml
@eddiew + @alanefl When you get the chance can you explain again why we wanted to change the signature for reduce/normalize? Changing the functions to accept Integers rather than mutable ClassElems doesn't affect whether unreduced elements exist; in both cases that guarantee depends on when reduce and normalize are called in client-facing operations (op, exp, etc.).
In the optimized branch @alanefl and I are working on passing in a new element by consuming its fields (a,b,c) rather than borrowing the element as mutable appears to require a few additional memory allocations, which would introduce a small performance overhead.
Eddie:
You may have checked already but is it possible to also use a macro to reduce test duplication (similar to how you use a macro for benchmark duplication)?
Yes, this is in the works. I opened an issue (#20) to track this.
also, don't forget to remove the dev flag from cargo.toml
Whoops, will do.
@mstraka100: let's discuss offline.
@eddiew I pushed a macro for unit test deduplication. I spent a good amount of time looking for alternatives, but this is the cleanest macro I could come up with. There are two limitations that prevented me from doing something nicer:
$function_name + $group_type
inside the macro.For the time being, I think the stuff I put here is OK.
For future reference, you use the macro like this:
test_all_groups!(
test_my_function,
test_my_function_rsa2048,
test_my_function_class,
// Add any other attributes you want on your unit test here as a comma-separated list
// e.g. should_panic(expected = "DivisionByZero"), should_whatever(expected = "Other")
);
fn test_my_function<G: UnknownOrderGroup>() {
// Your generic unit test.
}
Cool. Looks good!
Let's merge the current class groups implementation into master so that we can start benchmarking/testing/simulating over them. Optimization work for class groups is happening in different branches ATM.
Here's what in this PR:
cargo bench -- [rsa2048 | class]
. I wrote a macro to make this easy for benches, but not for tests -- there's an open issue for this.--features
.