Skip to content
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

fix: optimize compute_inner_product with parallel computation #37

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

crStiv
Copy link

@crStiv crStiv commented Jan 15, 2025

Description:

This PR optimizes the compute_inner_product function by implementing parallel computation for large vectors while maintaining efficient sequential computation for small ones.

Changes:

  • Implemented parallel computation for vectors larger than 32 elements
  • Kept sequential computation for small vectors to avoid parallelization overhead
  • Added comprehensive documentation explaining the optimization strategy
  • Added tests covering both sequential and parallel computation paths
  • Removed TODO comment as parallelization is now implemented

The optimization uses the existing parallelize infrastructure to split the computation into chunks and process them in parallel, which should improve performance for large vectors while maintaining efficiency for smaller ones.

Tests:

  • Added test cases for both small (16 elements) and large (64 elements) vectors
  • Tests use random data to ensure correctness
  • Tests verify results against sequential computation

This change maintains backward compatibility and follows the project's coding style.

Copy link
Collaborator

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use rayon sum instead for better performance.

@crStiv
Copy link
Author

crStiv commented Jan 15, 2025

@jonathanpwang this one is alright?

@jonathanpwang
Copy link
Collaborator

@jonathanpwang this one is alright?

your last commit seems to be empty

@crStiv
Copy link
Author

crStiv commented Jan 16, 2025

@jonathanpwang I dunno what's wrong with the previous one, but now it seems to be all alright

Copy link
Collaborator

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jonathanpwang
Copy link
Collaborator

Lints broken

@crStiv
Copy link
Author

crStiv commented Jan 17, 2025

Lints broken

@jonathanpwang
sorry for taking much of your time, but checks are green now and probably now everything should be fine

Copy link
Collaborator

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to include rayon = 1.8 in Cargo.toml
I will just merge and fix in followup

@jonathanpwang jonathanpwang merged commit 916c0bf into axiom-crypto:main Feb 3, 2025
0 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants