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

faster gcd #8410

Merged
merged 1 commit into from
Oct 7, 2014
Merged

faster gcd #8410

merged 1 commit into from
Oct 7, 2014

Conversation

nolta
Copy link
Member

@nolta nolta commented Sep 19, 2014

Average speedup for random inputs ranges from 20% 8% (Int8) to 70% (Int64).

@vtjnash
Copy link
Member

vtjnash commented Sep 19, 2014

is this actually a faster method, or just a way to avoid rem

@nolta
Copy link
Member Author

nolta commented Sep 20, 2014

If i'm reading this correctly, the binary algorithm should be about 1.8x faster for large N.

@carlobaldassi
Copy link
Member

This implementation requires trailing_zeros and bit shift operations, so it depends on the internal representation - I wouldn't expect all possible user-defined integer types to require implementing those functions, or conforming to a specific representation pattern. In other words, the bit representation shouldn't be part of the interface an Integer type is expected to have.

I think that in the interest of generality it would be useful to keep the old version (for user-defined integers), while this more optimized one could be specialized on the concrete Integer types of Base.

@nolta
Copy link
Member Author

nolta commented Sep 21, 2014

That's fair. I guess i'm a little squeamish about including code in Base which isn't used by any of the types in Base.

@carlobaldassi
Copy link
Member

Yes, I understand. It would also need to be tested explicitly via invoke or by constructing an Integer for that sole purpose, which is also somehow a possible hint of a problem.

One more issue is that of extending the specialized version: using a Union type would not be extensible, so a traits-based version would be better in that respect. But then we'd need to introduce a "standard binary representation" trait, or something along those lines.

On the other hand, gcd is a general function, which other functions depend upon (e.g. Rational constructor), and with a very clear mathematical meaning in terms of elementary operations, so I believe that the generic function with the abstract Integer signature should follow that.

Maybe having a concrete example in mind would be useful to further the discussion, e.g. ModInts.

@nolta
Copy link
Member Author

nolta commented Sep 22, 2014

Ok, i've restricted this to Union(64,128). The code is type unstable for ints < Int, so the speedup is only ~1.1x for random Int8/16/32, but then jumps to 1.7x for Int64 and 2.1x for Int128.

@eschnett
Copy link
Contributor

Type-unstable? Couldn't that be fixed rather easily by promoting the arguments or converting the result of trailing_zeros etc?

@nolta
Copy link
Member Author

nolta commented Sep 22, 2014

Sure, but i'm lazy.

@ivarne
Copy link
Member

ivarne commented Sep 22, 2014

Should we have a comment in the source to inform source code readers why we have two implementations of gcd for Integers? I don't think it is totally obvious.

@jiahao jiahao force-pushed the master branch 2 times, most recently from 2ef98c5 to 0388647 Compare October 5, 2014 00:57
@nolta
Copy link
Member Author

nolta commented Oct 7, 2014

Ok, comment added.

nolta added a commit that referenced this pull request Oct 7, 2014
@nolta nolta merged commit 215249d into master Oct 7, 2014
@nolta nolta deleted the mn/binarygcd branch October 7, 2014 03:11
@StefanKarpinski
Copy link
Member

Sure, but i'm lazy.

You have a strange definition of lazy.

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.

6 participants