STL_Extension: Use Boost Heap

[maxGimeno]

Summary of Changes

Replace the use of Modifiable_priority_queue with Boost::heap::fibonacci_queue.
EDIT: The reason is that Modifiable_priority_queue is an undocumented header that we want to avoid now that Boost provides a reliable replacement for it.

Release Management

• Affected package(s):AABB_tree, Polyline_simplifiaction_2, STL_Extension, Surface_mesh_simplification

[afabri]

Can you explain why you do this change? Deprecation of Modifiable_.., a fix of a bug, a convincing benchmark,....

[maxGimeno]

@afabri see the Edit in the description

[afabri]

@afabri see the Edit in the description

Can you please make a performance test, for example for surface mesh simplication.

[maxGimeno]

I used the test_edge_collapse_Polyhedron_3 with a 340 000 vertices (1919448 edges) chinese dragon, here are the results:
Before PR : 41.71s
After PR: 36.85s

[afabri]

I used the test_edge_collapse_Polyhedron_3 with a 340 000 vertices (1919448 edges) chinese dragon, here are the results:
Before PR : 41.71s
After PR: 36.85s

That's good news!!!

[sloriot]

Quick question: Instead of modifying the code of various packages, would it be possible to update only the implementation of Modifiable_priority_queue using the boost documented heap?

[maxGimeno]

I don't know. It wouldn't be trivial, that's all I can say without trying.

[sloriot]

With an external view, that's a bit surprising.

[afabri]

I guess what Sebastien want's to say that it is probably easy to use the fibonacci_heap as an implementation detail which can be de/activated with a #define, right as it is done now with a boost::relaxed_heap

[maxGimeno]

I looked back at the diff, it seems easier than I thought. Theproblem is we need to embark the map and maintain it, where it is not always needed, so there will be a loss of performance in Edge_collapse.

[sloriot]

My guess is that the use in polyline simplification was not optimal as those type of heaps are bad for contains query. In mesh simplification, the handle are already stored by the algo. So maybe the implementation of polyline simplification should be updated.

[afabri]

What are the total times of the executables? How often are they executed?

[maxGimeno]

You can check the code of the test to find that out.
They are executed 5 times each and the printed time is the average.

[maxGimeno]

• (there are alternative functions without the handles)

If they exist, they are not documented :/

[maxGimeno]

The map management is too costly, even with abseil's flat_hash_map.

[maxGimeno]

The map management is too costly, even with abseil's flat_hash_map.

@sloriot I think we decided that this PR was not to be integrated, because the benchmarks were really bad. Should we close that PR ?

[afabri]

@maxGimeno can you check if the new function std::map<>::extract() is a possible replacement.

[sloriot]

@afabri meant using std::map that has a new function in c++17 that could be used to update the priority. (Maybe you can keep the wrapper to simplify the testing?)

[maxGimeno]

so IIUC, the task is to design a heap based on a c++17 std::map<cost, type> to replace the mutable_queue_with_remove, use this custom heap internally to the Modifiable_priority_queue and finally retry the bench ?

[sloriot]

Something like that, maybe hidden behind a #ifdef CGAL_USE_FOOBAR

[maxGimeno]

Here I tried an implementation based on a cxx17 set, using the function extract() for update(), and it is still slower than the master version.

[sloriot]

Replaced by #6155