Skip to content

Speed up factor::numeric::gcd#1563

Merged
rivy merged 4 commits into
uutils:masterfrom
nicoonoclaste:factor/faster/gcd
Jul 25, 2020
Merged

Speed up factor::numeric::gcd#1563
rivy merged 4 commits into
uutils:masterfrom
nicoonoclaste:factor/faster/gcd

Conversation

@nicoonoclaste
Copy link
Copy Markdown
Contributor

  • Add a criterion-powered benchmark for factor::numeric::gcd.
  • Replace its implementation with Stein's binary GCD algorithm
    ~50-65% faster (on the GCD computations themselves)
  • Test against a reference implementation (Euclidean algorithm)

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

Can someone rerun CICD / Style (macos-latest, macos) ?
I still do not understand why we run the code style check on 3 different platforms, to be honest.

@rivy
Copy link
Copy Markdown
Contributor

rivy commented Jul 20, 2020

Can someone rerun CICD / Style (macos-latest, macos) ?

The GHA jobs will sometimes crash for various, seemingly chaotic, reasons. If the crashing job is just style, I'd ignore it and be about my day. For other jobs, you can use an amended commit (with no changes; git commit --amend --no-edit) and force push it to trigger a rebuild.

I still do not understand why we run the code style check on 3 different platforms, to be honest.

Different platforms expose/use different code.

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

I still do not understand why we run the code style check on 3 different platforms, to be honest.

Different platforms expose/use different code.

I'm aware, but that doesn't affect the code formatting and linting tools.

@rivy
Copy link
Copy Markdown
Contributor

rivy commented Jul 24, 2020

Different platforms expose/use different code.

I'm aware, but that doesn't affect the code formatting and linting tools.

I understand your point, but the "Style" check does frequently produce different warnings on different platforms (probably different compiler warnings; see "Annotations" for https://github.com/uutils/coreutils/actions/runs/177424066).

The function had to be made `pub`, this is a [known limitation] of Criterion.

[known limitation]: https://bheisler.github.io/criterion.rs/book/user_guide/known_limitations.html
Also add a property-based test against the Euclidean implementation.

numeric::gcd got ~50-65% faster, according to criterion. The effect on the
overall system is small, but later PRs will use a lot more GCD computations.
@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

I understand your point, but the "Style" check does frequently produce different warnings on different platforms

Exciting :(

Anyhow, this discussion is probably way out of scope for this PR, but I should mention I did rebase the branch now that #1554 landed. :)

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

Build failed on AppVeyor (Rust stable, Windows x64, GNU ABI) because the DNS resolution for static.rust-lang.org failed...

@nicoonoclaste
Copy link
Copy Markdown
Contributor Author

nicoonoclaste commented Jul 25, 2020

I made cargo fmt happy (accidentally put 2 use statements out-of-order when resolving the conflict)

@rivy rivy merged commit a6d7379 into uutils:master Jul 25, 2020
@nicoonoclaste nicoonoclaste deleted the factor/faster/gcd branch July 25, 2020 23:49
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