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

Modular exponentiation, modular multiplicative inverse, #11

Merged
merged 31 commits into from Nov 3, 2018

Conversation

Projects
None yet
4 participants
@NAThompson
Copy link
Contributor

NAThompson commented Jan 28, 2018

extended Euclidean algorithm, discrete logarithm.

A couple of design decision left:

Should we stick with boost::optional or use std::optional?

Should John's blazing fast integer square root be taken out of multiprecision and placed in boost.integer?

How can we test a much greater range of input without killing the CI system?

NAThompson added some commits Jan 28, 2018

[ci skip] Modular exponentiation, modular multiplicative inverse, ext…
…ended Euclidean algorithm, discrete logarithm.
[ci skip] Use less verbose naming. Add asserts as verfication of algo…
…rithms is a negligible fraction of total runtime. Use boost::multiprecision::powm and boost::multiprecision::sqrt rather than one-offs.
[ci skip] Add test of short int to see if there's any obvious places …
…for overflow (none are obvious, but no guarantees they still aren't there). Print basic information about the test to console so that failures are easier to track down.
[ci skip] It is *not* the case that a discrete log exists when the ba…
…se and modulus are coprime. Take 4^x = 2 mod 5 as a counterexample. Change API accordingly.
@Lastique

This comment has been minimized.

Copy link
Member

Lastique commented Oct 24, 2018

A few quick comments:

  • Don't use Boost.Format, its dependency is unnecessary. Use simple string operations to compose error messages, or std::ostringstream if something complicated needs to be done.
  • Use BOOST_THROW_EXCEPTION to throw exceptions. #include <stdexcept>.
  • Use SFINAE to limit template parameters instead of hard failing with static asserts.
  • Boost.Integer is currently a C++03 only library. Generally, I don't mind adding bits that require C++11 or later, but I'm not sure it is absolutely required in all cases in this PR. In any case, if C++11 and later components are added, their minimum C++ requirements should be documented.
  • I'm not sure about the question regarding multiprecision as I'm not using it and not sure what is the functional division between Boost.Integer and Boost.Multiprecision.

@NAThompson NAThompson changed the title [ci skip] Modular exponentiation, modular multiplicative inverse, ext… Modular exponentiation, modular multiplicative inverse, Oct 24, 2018

NAThompson added some commits Oct 24, 2018

Remove dependency on boost.format, remove unfettered use of auto in o…
…rder to move towards C++03 compatibility, use BOOST_THROW_EXCEPTION.
@NAThompson

This comment has been minimized.

Copy link
Contributor Author

NAThompson commented Oct 24, 2018

Don't use Boost.Format, its dependency is unnecessary. Use simple string operations to compose error messages, or std::ostringstream if something complicated needs to be done.

Good point, done.

Use BOOST_THROW_EXCEPTION to throw exceptions. #include .

Good point, done.

Use SFINAE to limit template parameters instead of hard failing with static asserts.

The problem is that there won't be substitution failure. The worst case is the extended Euclidean algorithm, where the solution only exists for signed integer types, but no operation required by the algorithm is undefined for unsigned types. The result is just nonsense.

As for the other cases checking that the type is integral, I'm less passionate about that. Presumably someone could get it to compile with a floating point type or over some ring, but maybe that's on them.

Boost.Integer is currently a C++03 only library. Generally, I don't mind adding bits that require C++11 or later, but I'm not sure it is absolutely required in all cases in this PR. In any case, if C++11 and later components are added, their minimum C++ requirements should be documented.

I've #included the <tuple> header for the extended Euclidean algorithm; arguably this is unnecessary and error prone and I should really be returning a generic struct. I've yet to find a good name for this so it'll have to wait for now. I've also used std::unordered_map from C++11; again, arguably I could use boost::unordered_map. That would make the dependency graph more complicated and presumably increase the maintenance burden. It looks like boost::unordered is not getting new algorithms now, and is only being updated to keep the build alive.

I'm not sure about the question regarding multiprecision as I'm not using it and not sure what is the functional division between Boost.Integer and Boost.Multiprecision.

I think the correct long-term decision is to move the multiprecision sqrt functionality into integer, but for now I feel like it's not a showstopper.

NAThompson added some commits Oct 25, 2018

Return custom struct from extended Euclidean algorithm rather than tu…
…ple. Reduce number of operations for tests to reduce CI system workload. Disable discrete log tests until we have time to figure out why they are failing.
[ci skip] Trade out algorithm from 'The Joy of Factoring' to Wikipedi…
…a's version which reduces the number of required temporaries. In fact, the speedup is not large, but the code is more compact, and for larger types, the difference becomes more noticeable.
a*p % m may overflow, do not perform naive multiplication in unit tes…
…ts or undefined behavior may result. [CI SKIP]
@NAThompson

This comment has been minimized.

Copy link
Contributor Author

NAThompson commented Oct 26, 2018

For the modular inverse and Extended euclidean algorithm, I have performed the following checks:

  • Run every single combination of 16 bit inputs and verified that the results are correct. Obviously I can't push this into the CI system or it'd die.
  • Run a vast number of 32 bit inputs (overnight), and verified that the results are correct.
  • Run a vast number of 64 and 128 bit inputs, and verified that the results are correct.
  • Run all the tests under UBSAN, and demonstrated no undefined behavior is detected by this tool.
  • Made sure it compiles with 16, 32, and 64 bit standard integer types, as well as 128 and 256 bit boost.multiprecision integer types.
  • Ensured that the cppcheck static analysis was clean.
  • Wrote a benchmark (not committed to the repo, since it uses googlebenchmark) and found the following performance characteristics on a 2.7GHz intel laptop:
----------------------------------------------------------------------
Benchmark                               Time           CPU Iterations
----------------------------------------------------------------------
BM_ModInverse<int32_t>                133 ns        133 ns    5614598
BM_ModInverse<int64_t>                137 ns        137 ns    5497224
BM_ModInverse<int128_t>               412 ns        411 ns    1638546
BM_ModInverse<int256_t>              1027 ns       1024 ns     744420
BM_ExtendedEuclidean<int32_t>         129 ns        129 ns    6226207
BM_ExtendedEuclidean<int64_t>         133 ns        133 ns    5983162
BM_ExtendedEuclidean<int128_t>        394 ns        393 ns    1719817
BM_ExtendedEuclidean<int256_t>        970 ns        968 ns     755801
  • I was unable to regenerate integer/doc/html/index.html. But I was able to generate an xml file which had my required changes.

We'll see if I can get the discrete log working; it appears that it's a bit more subtle than I first thought.

@NAThompson

This comment has been minimized.

Copy link
Contributor Author

NAThompson commented Oct 27, 2018

Ok, this PR is finished, but the CI system needs a lot of TLC.

@Lastique

This comment has been minimized.

Copy link
Member

Lastique commented Oct 29, 2018

I've updated CI configs, could you rebase your PR over the current develop?

@NAThompson

This comment has been minimized.

Copy link
Contributor Author

NAThompson commented Oct 29, 2018

I've updated CI configs, could you rebase your PR over the current develop?

Done, fingers crossed that the build will succeed. Do you understand the purpose of the rest of these tests? Some just generate huge numbers of warnings, so it might be worth considering their value and potentially removing them.

@Lastique

This comment has been minimized.

Copy link
Member

Lastique commented Oct 29, 2018

Do you understand the purpose of the rest of these tests?

I don't. I have very little knowledge about the library beyond that it allows to construct an integer type by size in bits.

Looking at the current PR, maybe we should move the definition of DISABLE_MP_TESTS to a common header?

Other than that and provided that the tests pass, do you consider the PR done?

@Lastique

This comment has been minimized.

Copy link
Member

Lastique commented Oct 29, 2018

I forgot about this:

Use SFINAE to limit template parameters instead of hard failing with static asserts.

The problem is that there won't be substitution failure. The worst case is the extended Euclidean algorithm, where the solution only exists for signed integer types, but no operation required by the algorithm is undefined for unsigned types. The result is just nonsense.

As for the other cases checking that the type is integral, I'm less passionate about that. Presumably someone could get it to compile with a floating point type or over some ring, but maybe that's on them.

My point was that having a hard failure via a static assert is rarely useful. If an algorithm is only usable with a certain subset of types (e.g. signed integers) then the corresponding template parameter should be constrained as such so that the algorithm is not callable if the requirements are not met. This allows to overload the algorithm for other types that might still make sense with the algorithm (e.g. multiprecision types) but require a different implementation. A hard failure prevents this.

Of course, it may be too difficult or expensive to check for the type constraints correctly. In which case you would normally only document the requirements but not enforce them in the code (neither by SFINAE nor by static asserts). Of course, this is a less preferred approach, but it is still better than static asserts because it doesn't fail in compile time if the template parameters happen to match the requirements.

@Lastique

This comment has been minimized.

Copy link
Member

Lastique commented Oct 29, 2018

I've also used std::unordered_map from C++11; again, arguably I could use boost::unordered_map.

std::unordered_map would be fine as long as C++11 requirement was documented. But I can see it is no longer used.

@NAThompson

This comment has been minimized.

Copy link
Contributor Author

NAThompson commented Oct 29, 2018

Other than that and provided that the tests pass, do you consider the PR done?

Yes. The algorithm really hasn't changed at all in the last 10 or so commits, it's all fighting the CI system.

If an algorithm is only usable with a certain subset of types (e.g. signed integers) then the corresponding template parameter should be constrained as such so that the algorithm is not callable if the requirements are not met.

This is something I have never done before; I just read about constraining template parameters, and the only example I've seen is how to constrain the template to a pointer type. Do you know how to constrain a template to a signed type without a static assert? I guess I could initialize a temporary to -1.

@Lastique

This comment has been minimized.

Copy link
Member

Lastique commented Oct 29, 2018

Do you know how to constrain a template to a signed type without a static assert?

For example, like so:

// Accepts a signed number-like type and returns a value of return_type
template< typename T >
typename boost::enable_if_c<
    std::numeric_limits< T >::is_specialized && std::numeric_limits< T >::is_signed,
    return_type
>::type
foo(T arg)
{
    return ret_val; // the return type is return_type
}

This should be callable for any signed number-like types, which have std::numeric_limits specialized. This includes fundamental C++ types, as well as Boost.Multiprecision types. If you want to lock the algorithm to just the fundamental integers, you can use boost::is_integral/std::is_integral in the condition.

If you're using C++11 already, you can use std::enable_if instead of boost::enable_if_c. The default type for return_type, if omitted, is void. See here.

Show resolved Hide resolved include/boost/integer/extended_euclidean.hpp Outdated
Show resolved Hide resolved test/extended_euclidean_test.cpp Outdated
Show resolved Hide resolved include/boost/integer/mod_inverse.hpp Outdated
Show resolved Hide resolved include/boost/integer/extended_euclidean.hpp Outdated
@Lastique

This comment has been minimized.

Copy link
Member

Lastique commented Nov 1, 2018

Looks like CI test only failed for gcc 4.7 in C++11 mode due to a problem in Boost.Multiprecision. I think we can disable Boost.Multiprecision on gcc 4.7 in C++11 mode for now. And could you report the problem to Multiprecision?

@jzmaddock

This comment has been minimized.

Copy link
Contributor

jzmaddock commented Nov 1, 2018

And could you report the problem to Multiprecision?

I've seen it, and I don't know how to fix it :(

@Lastique

This comment has been minimized.

Copy link
Member

Lastique commented Nov 1, 2018

@jzmaddock Does any other code branch at https://github.com/boostorg/multiprecision/blob/30bc41706d8e8a1cb7e7f060c075123acb4a164c/include/boost/multiprecision/number.hpp#L888 work with gcc 4.7? It seems, the compiler is struggling with boost::is_destructible, which is called by boost::is_constructible at https://github.com/boostorg/multiprecision/blob/30bc41706d8e8a1cb7e7f060c075123acb4a164c/include/boost/multiprecision/number.hpp#L910. Although imperfect, the other code branches do without it.

Show resolved Hide resolved test/mod_inverse_test.cpp Outdated
@jzmaddock

This comment has been minimized.

Copy link
Contributor

jzmaddock commented Nov 1, 2018

Does any other code branch at https://github.com/boostorg/multiprecision/blob/30bc41706d8e8a1cb7e7f060c075123acb4a164c/include/boost/multiprecision/number.hpp#L888 work with gcc 4.7?

Sure, but they either disable the explicit conversion operators, or use a simplified semi-broken version that will fail in other situations :(

One could ague that the issue is actually inside type_traits, but in reality it's gcc-4.7's pre-standard C++11 support. If the only issue is use of multiprecision types in optional then I'm inclined to settle for that as the least worse compromise for now.

@NAThompson

This comment has been minimized.

Copy link
Contributor Author

NAThompson commented Nov 3, 2018

Ok, are we all happy with this? It'd be nice to get it pushed past the finish line.

@Lastique Lastique merged commit aa68e17 into boostorg:develop Nov 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Lastique

This comment has been minimized.

Copy link
Member

Lastique commented Nov 3, 2018

Thanks, Nick.

@Lastique

This comment has been minimized.

Copy link
Member

Lastique commented Nov 4, 2018

@jzmaddock Apparently, the problem with Boost.Multiprecision and Boost.Optional is not only limited to gcc 4.7 but also any clang in C++03 mode:

https://travis-ci.org/boostorg/integer/builds/450332419

I think this is a serious problem that needs to be solved.

@NAThompson

This comment has been minimized.

Copy link
Contributor Author

NAThompson commented Nov 4, 2018

I think this is a serious problem that needs to be solved.

One workaround would be to just not use boost::optional<Z> as the return type from mod_inverse and instead return a Z. Non-existence of the modular inverse could be communicated by returning zero.

@Lastique

This comment has been minimized.

Copy link
Member

Lastique commented Nov 4, 2018

That wouldn't solve the problem users might still have with Boost.Multiprecision+Boost.Optional. Or any other combination that has the same properties.

I've created two PRs in both libraries which should resolve the problem on both sides. Let's see what the maintainers say.

@glenfe

This comment has been minimized.

Copy link
Member

glenfe commented on include/boost/integer/discrete_log.hpp in fc4d657 Dec 4, 2018

Did this introduce a new cycle? Optional depends on Integer indirectly. https://pdimov.github.io/boostdep-report/develop/module-levels.html (Optional depends on Utility depends on Container_hash depends on Integer).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.