search

CGAL / cgal

The public CGAL repository, see the README below
https://github.com/CGAL/cgal#readme
Other
4.97k stars 1.38k forks source link

Lazy Compute_hw_3 #5413

Open mglisse opened 3 years ago

mglisse commented 3 years ago

Issue Details

Noticed in #5379. For a lazy Cartesian kernel, Compute_hw_3 is unnecessarily expensive. It looks like it could return a singleton, like Lazy::zero but with 1. (it could even return static_one<FT> but that's getting more adventurous) Note that the availability of Compute_hx_3, etc in the lazy kernel prevents from having a Cartesian AK with a Homogeneous EK. I am not even sure it is safe to have both homogeneous...

Environment

GilesBathgate commented 3 years ago

https://github.com/CGAL/cgal/blob/74af3e7d33e7354e4090a1b17941a3f0c372ca8d/Cartesian_kernel/include/CGAL/Cartesian/function_objects.h#L1522-L1542

afabri commented 3 years ago

Do you have a concrete example where hw() is used. Then this code might be specialized for Cartesian/Homogenoeous, as I can't imagine that the compiler will optimize for example a multiplication or division with 1 away.

mglisse commented 3 years ago

Do you have a concrete example where hw() is used.

My report starts with a link to an issue that has such a concrete example (unless that issue is doing something wrong).

Then this code might be specialized for Cartesian/Homogenoeous,

That's what I suggested in that other issue.

as I can't imagine that the compiler will optimize for example a multiplication or division with 1 away.

It would for Simple_cartesian<double>, but indeed not for Lazy_kernel, unless this 1 is represented specially (my comment about static_one). However, that doesn't prevent from making hw() a bit less expensive than currently.

GilesBathgate commented 3 years ago

My report starts with a link to an issue that has such a concrete example (unless that issue is doing something wrong).

@mglisse Yes I agree, the way the code was it would use hw() for both cartesian and homogeneous kernels, however, the fix proposed for the issue alleviates the problem as you suggested.

I agree that optimising the cartesian kernel to return static one, cleans the code and makes it less expensive, but it would be good to see some microbenchmarks to see the impact.