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

Performance improvements - new return types in rust #57

Merged
merged 36 commits into from
Jun 9, 2024

Conversation

MusicalNinjaDad
Copy link
Owner

@MusicalNinjaDad MusicalNinjaDad commented Jun 9, 2024

Rust 4.0.0 & Python 3.0.1

  • BREAKING CHANGES - Rust: new return types for significant performance improvements:
    • MultiFizzBuzz now lazily returns a rayon IndexedParallelIterator
    • FizzBuzz returns a FizzBuzzAnswer which can be converted into a String or Cow<str>
    • FizzBuzzAnswer now represents the valid answers to FizzBuzz, not One(String)or Many(Vec>String>)
  • Python implementation updated to work with rust v4.0.0 which brings slight performance improvements (approx 10-20%)

Summary by Sourcery

This pull request introduces significant performance improvements by updating the return types for FizzBuzz and MultiFizzBuzz. The FizzBuzzAnswer enum has been refactored to directly represent valid FizzBuzz answers. The Python implementation has been updated to work with Rust v4.0.0, bringing slight performance improvements. Documentation and tests have been updated accordingly.

  • New Features:
    • Introduced new return types for FizzBuzz and MultiFizzBuzz to improve performance, including lazy evaluation with rayon::iter::IndexedParallelIterator.
  • Enhancements:
    • Updated FizzBuzzAnswer to represent valid FizzBuzz answers directly, removing the need for One(String) and Many(Vec<String>) variants.
    • Enhanced the Python implementation to work with Rust v4.0.0, resulting in slight performance improvements (approx 10-20%).
  • Documentation:
    • Updated documentation to reflect new return types and usage examples for FizzBuzz and MultiFizzBuzz.
  • Tests:
    • Added new tests to cover the updated FizzBuzzAnswer and MultiFizzBuzz implementations, including tests for negative numbers and non-whole numbers.
    • Updated existing tests to use the new return types and ensure compatibility with the new implementation.

@MusicalNinjaDad MusicalNinjaDad marked this pull request as ready for review June 9, 2024 07:30
Copy link

sourcery-ai bot commented Jun 9, 2024

Reviewer's Guide by Sourcery

This pull request introduces significant performance improvements by changing the return types in the Rust implementation of the FizzBuzz library. The MultiFizzBuzz trait now returns a lazy rayon::iter::IndexedParallelIterator, and the FizzBuzz function returns a FizzBuzzAnswer enum, which can be converted into a String or Cow<str>. The Python implementation has also been updated to work with Rust v4.0.0, resulting in slight performance improvements (approximately 10-20%).

File-Level Changes

Files Changes
rust/fizzbuzz/src/lib.rs
rust/fizzbuzz/tests/test_numbertypes.rs
rust/fizzbuzz/tests/test_multifizzbuzz.rs
Updated Rust implementation to use new return types for performance improvements and updated corresponding tests.
rust/fizzbuzzo3/src/lib.rs
tests/perf_results.md
Updated Python wrapper and added performance results for new return types.
rust/fizzbuzz/benches/bench_fizzbuzz.rs
rust/fizzbuzz/benches/bench_sizes.rs
Updated benchmarks to reflect new return types and added new benchmarks for Cow<str>.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

Copy link

codecov bot commented Jun 9, 2024

Codecov Report

Attention: Patch coverage is 94.33962% with 3 lines in your changes missing coverage. Please review.

Files Patch % Lines
rust/fizzbuzz/src/lib.rs 90.62% 3 Missing ⚠️
Additional details and impacted files
Flag Coverage Δ
fizzbuzz 94.64% <90.62%> (+0.79%) ⬆️
fizzbuzzo3 90.52% <81.25%> (+3.02%) ⬆️
python 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📢 Thoughts on this report? Let us know!

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @MusicalNinjaDad - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 7 issues found
  • 🟢 Security: all looks good
  • 🟡 Testing: 3 issues found
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

rust/fizzbuzz/src/lib.rs Show resolved Hide resolved
rust/fizzbuzz/src/lib.rs Outdated Show resolved Hide resolved
rust/fizzbuzz/src/lib.rs Outdated Show resolved Hide resolved
rust/fizzbuzz/src/lib.rs Show resolved Hide resolved
rust/fizzbuzz/src/lib.rs Show resolved Hide resolved
rust/fizzbuzz/src/lib.rs Outdated Show resolved Hide resolved
rust/fizzbuzz/benches/bench_fizzbuzz.rs Show resolved Hide resolved
@@ -32,7 +33,9 @@ macro_rules! test_this {
}

/// Test all compatible standard types
mod standard_types {
mod standard_types_up_to_127_as_strings {
Copy link

Choose a reason for hiding this comment

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

suggestion (testing): Add tests for boundary values

It would be useful to add tests for boundary values such as 0, 1, and the maximum values for each type to ensure the new return types handle these correctly.

rust/fizzbuzz/tests/test_multifizzbuzz.rs Show resolved Hide resolved
rust/fizzbuzz/tests/test_numbertypes.rs Show resolved Hide resolved
@MusicalNinjaDad MusicalNinjaDad merged commit 8f85294 into main Jun 9, 2024
21 checks passed
@MusicalNinjaDad MusicalNinjaDad deleted the MusicalNinjaDad/issue55 branch June 9, 2024 10:01
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.

1 participant