add optimistic inv #170
add optimistic inv #170
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @siq1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request aims to improve the performance of the compilation process by optimizing the calculation of field inverses. It introduces an 'optimistic' inverse function that handles the common cases of zero and one more efficiently, avoiding the potentially more expensive general inverse operation. This change is reported to yield significant compilation time improvements for certain circuits.
Highlights
- Performance Optimization: Introduced an
optimistic_invmethod on theFieldtrait. This method provides a faster path for calculating the inverse of field elements that are zero or one, which are expected to be frequent cases during compilation. - Apply Optimization: Replaced direct calls to
inv().unwrap()with the newoptimistic_inv().unwrap()in key areas of the compiler builder logic (basic.rsandfinal_build_opt.rs) where field inverses are calculated, leveraging the performance improvement.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
| fn optimistic_inv(&self) -> Option<Self> { | ||
| if self.is_zero() { | ||
| None | ||
| } else if *self == Self::ONE { | ||
| Some(Self::ONE) | ||
| } else { | ||
| self.inv() | ||
| } | ||
| } |
There was a problem hiding this comment.
Consider adding a comment explaining the purpose of this function, especially the 'optimistic' part. What are the assumptions and potential benefits?
// optimistic_inv returns Some(inverse) if self is not zero.
// It optimizes the case where self is ONE.
fn optimistic_inv(&self) -> Option<Self> {| } | ||
| None => { | ||
| let coef_inv = coef.inv().unwrap(); | ||
| let coef_inv = coef.optimistic_inv().unwrap(); |
There was a problem hiding this comment.
It's generally better to use expect instead of unwrap to provide a more informative error message in case of failure.
| let coef_inv = coef.optimistic_inv().unwrap(); | |
| let coef_inv = coef.optimistic_inv().expect("coef should not be zero"); |
| } | ||
| let v = e[0].coef; | ||
| let vi = v.inv().unwrap(); | ||
| let vi = v.optimistic_inv().unwrap(); |
There was a problem hiding this comment.
Using expect can provide a clearer error message if v.optimistic_inv() returns None.
| let vi = v.optimistic_inv().unwrap(); | |
| let vi = v.optimistic_inv().expect("v should not be zero"); |
| self.mid_var_coefs.push(MidVarCoef { | ||
| k: coef, | ||
| kinv: coef.inv().unwrap(), | ||
| kinv: coef.optimistic_inv().unwrap(), |
There was a problem hiding this comment.
Consider using expect for a more descriptive error message.
| kinv: coef.optimistic_inv().unwrap(), | |
| kinv: coef.optimistic_inv().expect("coef should not be zero"), |
| self.mid_var_coefs.push(MidVarCoef { | ||
| k: coef, | ||
| kinv: coef.inv().unwrap(), | ||
| kinv: coef.optimistic_inv().unwrap(), |
There was a problem hiding this comment.
Using expect can provide a clearer error message if coef.optimistic_inv() returns None.
| kinv: coef.optimistic_inv().unwrap(), | |
| kinv: coef.optimistic_inv().expect("coef should not be zero"), |
| } | ||
| let v = e[0].coef; | ||
| let vi = v.inv().unwrap(); | ||
| let vi = v.optimistic_inv().unwrap(); |
There was a problem hiding this comment.
Consider using expect for a more descriptive error message.
| let vi = v.optimistic_inv().unwrap(); | |
| let vi = v.optimistic_inv().expect("v should not be zero"); |
In compilation process,
inv(1)is called too many times. By adding the optimistic path, it gains 10%-20% improvement in compilation time of some circuits.