refactor: update the test_cs.rs #44
Conversation
|
I'm not sure what makes the test take so long |
because the origin modification to |
There was a problem hiding this comment.
@flyq, thanks for the contribution. Bellpepper-core is a load-bearing crate with significant dependencies, and aside from providing refactorings of existing methods, I have missed what this PR fixes (in your commits' own terms).
Because of that large production footprint, changes to bellpepper-core tend to be in "if it ain't broke, don't fix it" mode, and / or require strong evidence of significant improvement before getting in.
In other circumstances, your PR would have been welcome as an arguable improvement in readability, but the specific context of this crate makes accepting the change more difficult - unless I have missed a nuance about a major correctness or performance improvement brought by your change, of course. Please let me know if that's the case.
| .iter() | ||
| .filter(|(_, v)| **v != Scalar::ZERO) | ||
| .map(|(k, v)| (OrderedVariable(k), *v)) | ||
| .collect() |
There was a problem hiding this comment.
Fair enough, but since the TestCS type is used in (tests of) gigantic proving projects, the performance matters. Here the FromIterator implementation of BTreeMap is sorting the keys of the input before insertion, so I would need to see micro-benchmarks of this method against the prior implementation before considering this change (to compare your
|
Closing to make the status clear, but I'll track this issue for a few days, please do comment if you think there's something I missed. |
|
Yeah, as you said, it's just to improve code readability, the performance difference has not been rigorously tested. Keeping it as is is probably the best option |
No description provided.