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

refactor: Optimize instruction_accounts creation in invoke_context #2475

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

codebender828
Copy link

@codebender828 codebender828 commented Aug 7, 2024

Problem

PS: I am a fairly new contributor, and a student of the code base, so these changes may be reviewed with a grain of salt.

The current implementation, while functional, has room for improvement in terms of performance and readability. These changes aim to make the code more idiomatic and potentially more efficient, especially when dealing with large datasets or frequent calls.

Summary of Changes

This PR introduces a tiny optimization for the creation of instruction_accounts in the invoke_context file of the Anza client implementation for Solana. It also hopes to improve code readability, and adherence to general Rust idioms.

Changes

  • Replace explicit clone() with cloned() method
  • Reorder operations for more efficient error handling
  • Simplify the closure passed to map()

Benefits

  1. Improved Readability: The new version has a flatter structure in the closure, making it easier to understand at a glance.
  2. More Idiomatic Rust: Using cloned() instead of explicit clone() is more idiomatic and potentially more efficient.
  3. Optimized Error Handling: The error is now only created when necessary, potentially saving unnecessary allocations.

Future Optimizations

While not included in this PR, here are some ideas for future optimizations:

  1. If the size of duplicate_indicies is known in advance, pre-allocating the Vec could provide a small performance boost.
  2. If this operation is performed in a loop, consider using iterators throughout instead of collecting into a Vec, if possible.

Performance Impact

While the performance difference is likely minimal for small datasets or infrequent calls, it could be noticeable for large-scale operations. Benchmarking is recommended to quantify the improvement.

Please review and let me know if you have any questions or suggestions for further improvements!

@mergify mergify bot requested a review from a team August 7, 2024 17:25
@codebender828 codebender828 marked this pull request as ready for review August 7, 2024 17:36
@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Aug 7, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Aug 7, 2024
Copy link

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Definitely cleaner and rust-ier. Thanks for the contribution!

@CriesofCarrots CriesofCarrots merged commit 169eeec into anza-xyz:master Aug 8, 2024
44 checks passed
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
anza-xyz#2475)

refactor: optimize instruction_accounts creation in invoke_context
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants