Enable Boost.Multiprecision with MSVC and recent boost

[mglisse]

Summary of Changes

In #3406, the plan was

• disable boost.multiprecision for MSVC for CGAL-4.14,
• and reenable it for 4.15, after 1.70 has been released.

But apart from a reminder in #4495 it was forgotten.

This branch is completely untested, the whole point is for the testsuite to run on it (I checked and there is at least one platform with msvc and a recent enough boost).

Release Management

• Affected package(s): Number_types

[afabri]

What do you have in mind with polishing when you write
// This is disabled for now because cpp_rational is even slower than Quotient<MP_Float>. Quotient<cpp_int> will be a good candidate after some polishing.

here

[mglisse]

What do you have in mind with polishing

Probably what is discussed in #3614 (mentioned on the previous line), in particular to_double, possibly to_interval.

[afabri]

What do you have in mind with polishing

Probably what is discussed in #3614 (mentioned on the previous line), in particular to_double, possibly to_interval.

As this PR is merged the polishing is done?

[mglisse]

As this PR is merged the polishing is done?

I don't think so.

[lrineau]

The polishing you are both talking about, is it the content of specializations of CGAL::Real_embeddable_traits<T> and CGAL::Algebraic_structure_traits<T>? Or are we talking about something else completely?

[mglisse]

If it is mostly to_double / to_interval for Quotient, they do appear to be part of Real_embeddable_traits.

[lrineau]

@afabri:

• Have you tried this PR on Windows?
• Do we want to wait for the resolution of the discussion about Quotient<cpp_int>, or can we begin the testing/integration of the PR?

[lrineau]

Note that you need Boost>=1.70 to see the benefits.

[afabri]

Is CGAL::Gmpq still hard wired in classes such CGAL::Epeck, or is the Excact_type_selector just about replacing our usage of CGAL::Gmpq, unless we explicitely write something like Simple_cartesian<Gmpq>?

[lrineau]

Is CGAL::Gmpq still hard wired in classes such CGAL::Epeck, or is the Exact_type_selector just about replacing our usage of CGAL::Gmpq, unless we explicitely write something like Simple_cartesian<Gmpq>?

You know that we have users who can run CGAL code without GMP at all (and with or without LEDA). We did the work, using Exact_type_selector, for those users. Gmpq is not hardcoded.

[mglisse]

* Do we want to wait for the resolution of the discussion about `Quotient<cpp_int>`

Please don't, those are separate issues.

@afabri Epick and Epeck use Exact_field_selector.

[afabri]

So this means BEFORE this PR for non-windows users we have non-reference counted GMP and for Windows users reference counted ?
This should be visible for example when we benchmark Boolean operations.

[mglisse]

I don't believe there was any announcement in CHANGES.md when we switched to gmpxx/boost_mp on linux and mac. Do you expect one for windows?

[lrineau]

I don't believe there was any announcement in CHANGES.md when we switched to gmpxx/boost_mp on linux and mac. Do you expect one for windows?

Yes, because that was an error not to document that change for linux and mac.

[mglisse]

On windows, the type used for Exact_rational, in Epick and indirectly (through Lazy_exact_nt) Epeck may now be boost::multiprecision::mpq_rational, as has been the case on other platforms for several releases. This depends on various options and is added to a list that includes mpq_class, CGAL::Gmpq, leda_rational and CGAL::Quotient<CGAL::MP_Float>.

If you want to document that somewhere, feel free to add something to the doc of Epeck or Exact_rational in a separate PR.

[afabri]

Can it really be CGAL::Quotient<CGAL::MP_Float> with some options?

[mglisse]

Can it really be CGAL::Quotient<CGAL::MP_Float> with some options?

-DCGAL_DISABLE_GMP and without LEDA. That's the case we are talking of switching to Quotient<cpp_int> later.

[afabri]

@afabri:

* Have you tried this PR on Windows?

* Do we want to wait for the resolution of the discussion about `Quotient<cpp_int>`, or can we begin the testing/integration of the PR?

In Simon's arrangement benchmark it is slightly slower.
EDIT: Simon could not reproduce it on Linux.

[lrineau]

@afabri:

* Have you tried this PR on Windows?

* Do we want to wait for the resolution of the discussion about `Quotient<cpp_int>`, or can we begin the testing/integration of the PR?

In Simon's arrangement benchmark it is slightly slower.

We should redo the benchs on Linux too, then.

[mglisse]

In Simon's arrangement benchmark it is slightly slower.

Which one is that? Is it in benchmark/Arrangement_on_surface_2?

[mglisse]

We should redo the benchs on Linux too, then.

Indeed. IIRC, the kind of test that was run last time was more like Orientation_3 with Simple_cartesian where Q was Gmpq or mpq_class.

Note that I would still be interested in a run of the testsuite to check that it works on windows. Then we could at least enable boost_mp on windows, even if we don't make it the default.

[afabri]

If it is mostly to_double / to_interval for Quotient, they do appear to be part of Real_embeddable_traits.

I don't understand this sentence. Can you please elaborate. What has to be done (in order to achieve what)?

[mglisse]

I once heard that for double it is better to copy than to const&. Is that so, or just hearsay?

It is so, but irrelevant if the function is inlined, and if the function is not inlined the function is likely expensive and the cost of the reference negligible.

We have Is_zero and is_zero() but I could not find a special version for Gmpq.

We have one for mpq_class and mpq_rational, and could easily add one for Gmpq. But also, a-b==0 looks like a complicated way to say a==b...
I also believe we should require NT to be interoperable with int (they all are AFAIK) and stop writing ==FT(0), /FT(2), etc which allocate a FT unnecessarily.

And the maybe most important question is if we want to optimize for Simple_cartesian<Exact_rational> or not better focus on Epeck.

I believe we should focus on Epeck. But it isn't clear if the default rational type in Epeck should be the same as the default rational type for other uses.

The former can still help to find copies.

👍 that's why I was testing it 😉

[jzmaddock]

I also believe we should require NT to be interoperable with int (they all are AFAIK) and stop writing ==FT(0), /FT(2), etc which allocate a FT unnecessarily.

You probably know this already, but for Boost.Multiprecision all the number types are required to interoperate with all other number types (obviously including integer literals) as long as the implied implicit conversion is not "lossy". Generally speaking, these mixed operations are significantly faster than constructing a temporary. There is one - perhaps somewhat theoretical - exception: if one argument to an operation is an rvalue, then we can reuse that variable as working storage during the operation. It's often tricky to program without introducing subtle bugs though, so it's not actually used that much currently.

[afabri]

But also, a-b==0 looks like a complicated way to say a==b...

Well, you misrepresent a little bit, because the a-b is needed later if the intersection is a point, which is almost always the case.

[afabri]

stop writing ==FT(0)

Don't we have static variables zero sometimes. But is_zero() is probably even better.

[mglisse]

Well, you misrepresent a little bit, because the a-b is needed later if the intersection is a point, which is almost always the case.

The compiler cannot CSE (common sub-expression) code using rationals. If the same computed value is reused, it should be stored in a variable, not computed twice.

[afabri]

Well, you misrepresent a little bit, because the a-b is needed later if the intersection is a point, which is almost always the case.

The compiler cannot CSE (common sub-expression) code using rationals. If the same computed value is reused, it should be stored in a variable, not computed twice.

We have a variable here
You are right for what happens two lines below, but parallel lines will be rare.

[afabri]

Btw, what about making CGAL::Mpzf depending on boost::mp?

That would be possible, probably not very hard, although I expect the gains would be rather small (but I cannot say for sure without trying).

So what is used for functors which are division free, if GMP is not installed?

[mglisse]

So what is used for functors which are division free, if GMP is not installed?

MP_Float

template <>
struct Exact_ring_selector<double>
#ifdef CGAL_HAS_MPZF
{ typedef Mpzf Type; };
#elif defined(CGAL_HAS_THREADS) || !defined(CGAL_USE_GMP)
{ typedef MP_Float Type; };
#else
{ typedef Gmpzf Type; };
#endif

[afabri]

So what is used for functors which are division free, if GMP is not installed?

MP_Float

template <>
struct Exact_ring_selector<double>
#ifdef CGAL_HAS_MPZF
{ typedef Mpzf Type; };
#elif defined(CGAL_HAS_THREADS) || !defined(CGAL_USE_GMP)
{ typedef MP_Float Type; };
#else
{ typedef Gmpzf Type; };
#endif

So is the intuition that MP_Float is fast (and therefore maybe even pretty promising in a Quotient) because it is trivial to construct from a double?

[mglisse]

So is the intuition that MP_Float is fast (and therefore maybe even pretty promising in a Quotient) because it is trivial to construct from a double?

MP_Float is the last (slowest) choice here for when we cannot use Mpzf or Gmpzf. It is still faster than using a rational, since +-* operations on MP_Float are just that and don't involve a reduction to the same denominator like a rational, but if the other two are available, they are likely better than MP_Float for all purposes.

The fact that a MP_Float (like Mpzf and Gmpzf) can represent a double does give Quotient<MP_Float> some advantage over Quotient<integer> because after conversion the denominator is 1 (and the numerator is a small integer), which can make subsequent operations a bit faster. More generally, the exponent can help when the numerator or denominator of a fraction of integers would be a multiple of 2 with large multiplicity, which happens often in CGAL because of conversions from double. MP_Float is essentially integer * 2^exp, and in a Quotient the 2 exponents are redundant; it would be slightly better to have integer / integer * 2^exp, but it probably wouldn't change much. Note that the integer part of MP_Float is not very fast.

[mglisse]

I reenabled boost_mp without GMP in this same branch, because it affects the same line, and both changes try to go back to what #3406 wanted.
In a quick test (NewKernel_d) with CGAL_DISABLE_GMP, it works strictly better, since it now compiles while it doesn't otherwise (see #4319).

[afabri]

Btw, in a mail exchange with @jzmaddock, (co-author of boost::mp) about what to improve he wrote:

If we're talking about cpp_rational, the next low hanging fruit, would probably be to ditch the current rational_adaptor implementation in favor of one which doesn't depend on Boost.Rational: both to reduce the number of temporaries used and for because for arbitrary precision integers there's a lot of error checking code in Boost.Rational that's not required anymore.

@jzmaddock can you in the code point out where the temporaries are you mention, as well the error checking code?

[maxGimeno]

Successfully tested in https://cgal.geometryfactory.com/CGAL/testsuite/results-5.3-Ic-34.shtml

[lrineau]

Btw, in a mail exchange with @jzmaddock, (co-author of boost::mp) about what to improve he wrote:

If we're talking about cpp_rational, the next low hanging fruit, would probably be to ditch the current rational_adaptor implementation in favor of one which doesn't depend on Boost.Rational: both to reduce the number of temporaries used and for because for arbitrary precision integers there's a lot of error checking code in Boost.Rational that's not required anymore.

@jzmaddock can you in the code point out where the temporaries are you mention, as well the error checking code?

@mglisse @afabri, Maxime just marked this PR as successfully "Tested" in the testsuite, but it seems there is still an ongoing discussion. Should I merge the PR as it is, or wait?

[mglisse]

The discussions about cpp_rational are unrelated to this PR.
We should merge this PR, since the tests now pass on windows.
This has the side effect of changing the default rational type on windows from Gmpq to mpq_rational (which was already the default on other platforms), which can have positive or negative speed consequences depending on the package. Whether you want to block this PR until it is decided which one should be the default (if it changes, it should be in a separate PR in my opinion, and that choice is not irreversible, it can be revisited regularly) or not is your choice.