perf(math): Add #[inline] attributes to hot field arithmetic paths#1128
Merged
diegokingston merged 2 commits intomainfrom Feb 3, 2026
Merged
perf(math): Add #[inline] attributes to hot field arithmetic paths#1128diegokingston merged 2 commits intomainfrom
diegokingston merged 2 commits intomainfrom
Conversation
Add inline attributes to performance-critical field arithmetic functions to improve optimization opportunities for the compiler: - element.rs: Add #[inline] to From conversions, PartialEq, Div trait implementations, Default, from_raw(), and to_raw() - mersenne31/field.rs: Add #[inline(always)] to all IsField and IsPrimeField implementations including add, sub, mul, neg, inv, div, eq, zero, one, from_u64, from_base_type, double, representative. Also add inline to helper methods weak_reduce, as_representative, mul_power_two, pow_2, two_square_minus_one. - u64_goldilocks_field.rs: Add #[inline] to inv and div for both Goldilocks64Field and Degree2GoldilocksExtensionField, plus #[inline(always)] to eq, zero, one for the extension field. These inline hints help the compiler make better inlining decisions for small, frequently-called functions in hot loops like FFT and polynomial operations.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #[inline(always)] | ||
| fn representative(x: &u32) -> u32 { | ||
| debug_assert!((x >> 31) == 0); | ||
| Self::as_representative(x) |
There was a problem hiding this comment.
Correctness
- The
mul_power_twoimplementation appears to be incorrectly handling the shifting of bits, leading to possible incorrect multiplication results when the bits carried exceed the maximum allowed. - The function
pow_2should ensure that squaring does not overflow, as continued squaring can exceed the 31-bit expectation.
Security
- Functions like
invshould ensure no secret-dependent branching for side-channel prevention; similar care is needed with functions likemul,sub, andnegfor consistent timing. - The
as_representative,neg, andinvmethods may benefit from checking their handling of inputs like zero and the prime field order.
Performance
- Excessive
#[inline(always)]attributes may lead to code bloat; use them judiciously, especially for complex logic. - Review the
mulconversion fromu32tou64and back to ensure minimal overhead and evaluate if it's performant enough.
Bugs & Errors
- The
subfunction uses a subtraction which could underflow ifa < b. - Potential overflows in arithmetic functions considering operations return values larger than 31 bits which may lead to crashes.
Code Simplicity
- The
mul_power_twoimplementation lacks clarity and would benefit from comments or a simplified approach. - Multiple uses of
weak_reduceimply a potential for consolidation or more straightforward flow; repeated usage may cover duplicated logic.
Additional Notes
- Ensure rigorous testing for edge cases, such as successive operations leading to zero or the prime field order, especially in reduced field context.
- Consider adding comments or documentation to clarify assumptions, such as when bitwise operations are safe and why certain methods may not need full reductions.
Due to these concerns in correctness, security, performance, and potential bugs, this code is not ready to merge. Further refinement and testing are required to ensure all conditions are safely managed and optimized.
| #[inline(always)] | ||
| fn one() -> Self::BaseType { | ||
| [FpE::one(), FpE::zero()] | ||
| } |
There was a problem hiding this comment.
Correctness:
invfunction: The handling of zero ininvseems correct, as it returns an error when the input is zero, which is an expected behavior to prevent division by zero.
Security:
- There is no indication in the provided snippet whether the operations are constant-time. Ensure that modular arithmetic and field operations are constant-time to prevent timing side-channel attacks.
- Make sure that sensitive data is properly zeroized after use. I couldn't see if and where sensitive data is zeroized from the snippet.
- There's no detail about randomness in this snippet, so it should be secured elsewhere if used.
- Ensure hash function domain separation where applicable.
Performance:
- Inlining: Adding
#[inline]and#[inline(always)]hints is good for performance, but be cautious as#[inline(always)]can sometimes lead to code bloat, though this seems appropriate here. - Multiplicative Inverse: The implementation uses addition chains for inverse calculation, which is often performance-optimal.
Bugs & Errors:
- Error handling: The usage of
Resulttypes for inversion and division functions is appropriate to handle errors such as division by zero. - Potential Panics/Unwraps: There are no obvious uses of
unwrapfrom the snippet provided. Ensure any other parts of the code do not invokeunwrapwithout checks. - Memory Safety: Without the broader context, there's nothing that appears unsafe here, but ensure all buffer/array accesses are bounds-checked.
Code Simplicity:
- The code is concise and focuses on using existing utility functions like
invandmul, which promotes readability and maintainability. - Using descriptive names like
norm_inv,a0_sq, anda1_sqaids in understanding the algorithm.
Recommendation:
- Ensure you perform all cryptographic operations, especially those involving secrets, in constant time.
- Double-check that all potential error conditions are appropriately handled, especially when the code relies on external inputs or parameters.
- Make sure sensitive data is zeroed out after use, especially in areas handling private keys or other secrets.
Kimi AI ReviewThe PR diff introduces several changes, primarily focused on adding
|
Greptile OverviewGreptile SummaryThis PR adds Changes:
Minor inconsistency: Extension field's Confidence Score: 4.5/5
|
| Filename | Overview |
|---|---|
| crates/math/src/field/element.rs | Added inline attributes to trait implementations (From, PartialEq, Div, Default) and utility methods (from_raw, to_raw) to improve compiler optimization for hot paths |
| crates/math/src/field/fields/mersenne31/field.rs | Added inline(always) to all IsField/IsPrimeField trait methods and helper functions to optimize hot arithmetic paths for Mersenne31 field operations |
| crates/math/src/field/fields/u64_goldilocks_field.rs | Added inline attributes to inv/div for base and extension fields, and inline(always) to eq/zero/one for extension field, though some methods remain without inline |
Sequence Diagram
sequenceDiagram
participant User as User Code
participant FE as FieldElement
participant Field as Field Implementation
participant Compiler as Rust Compiler
Note over User,Compiler: Hot Path Field Arithmetic
User->>FE: element1 + element2
activate FE
FE->>Field: F::add(&val1, &val2)
activate Field
Note over Field: inline(always) forces inlining
Field-->>FE: result (inlined)
deactivate Field
FE-->>User: FieldElement (optimized)
deactivate FE
User->>FE: element.inv()
activate FE
Note over FE: inline(always) wrapper
FE->>Field: F::inv(&value)
activate Field
Note over Field: inline allows compiler discretion
Field->>Field: Complex inv logic
Field-->>FE: Result value
deactivate Field
FE-->>User: Result FieldElement
deactivate FE
User->>FE: From trait conversion
activate FE
Note over FE: inline on From impl
FE->>Field: F::from_base_type(x)
activate Field
Note over Field: inline(always) on field method
Field-->>FE: converted value (fully inlined)
deactivate Field
FE-->>User: FieldElement (zero overhead)
deactivate FE
Note over User,Compiler: Result: Reduced call overhead, better register allocation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Additional Comments (1)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/math/src/field/fields/u64_goldilocks_field.rs
Line: 451:461
Comment:
Missing `#[inline(always)]` on `from_u64`, `from_base_type`, and `double` for consistency with base `Goldilocks64Field` (lines 170-189) and to complete hot path optimization
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1128 +/- ##
=======================================
Coverage 71.33% 71.33%
=======================================
Files 166 166
Lines 36072 36072
=======================================
Hits 25732 25732
Misses 10340 10340 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Kimi AI ReviewHere are my feedback and comments on the PR diff:
|
jotabulacios
approved these changes
Feb 3, 2026
Contributor
|
Benched it with hyperfine (10M iterations of add/mul/sub loop) and
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add inline attributes to performance-critical field arithmetic functions to improve optimization opportunities for the compiler:
element.rs: Add #[inline] to From conversions, PartialEq, Div trait implementations, Default, from_raw(), and to_raw()
mersenne31/field.rs: Add #[inline(always)] to all IsField and IsPrimeField implementations including add, sub, mul, neg, inv, div, eq, zero, one, from_u64, from_base_type, double, representative. Also add inline to helper methods weak_reduce, as_representative, mul_power_two, pow_2, two_square_minus_one.
u64_goldilocks_field.rs: Add #[inline] to inv and div for both Goldilocks64Field and Degree2GoldilocksExtensionField, plus #[inline(always)] to eq, zero, one for the extension field.