[SofaKernel] Switch from map_ptr_stable_compare to std::unordered_map

[alxbilger]

This is a discussion about the need for sofa::helper::map_ptr_stable_compare, in NarrowPhaseDetection, but also in Sofa in general (it is also used in DefaultContactManager).

According to benchmarks comparing 3 different types of maps, sofa::helper::map_ptr_stable_compare is several orders of magnitude slower than std::map and std::unordered_map.

std::unordered_map

------------------------------------------------------------------------------------------
Benchmark                                                Time             CPU   Iterations
------------------------------------------------------------------------------------------
BM_NarrowPhaseDetection_getDetectionOutputs/8         19.8 us         19.7 us        32412
BM_NarrowPhaseDetection_getDetectionOutputs/16        42.8 us         42.8 us        16528
BM_NarrowPhaseDetection_getDetectionOutputs/32         121 us          121 us         5764
BM_NarrowPhaseDetection_getDetectionOutputs/64         401 us          401 us         1750
BM_NarrowPhaseDetection_getDetectionOutputs/128       2228 us         2228 us          316
BM_NarrowPhaseDetection_getDetectionOutputs/256       9252 us         9253 us           75

std::map

------------------------------------------------------------------------------------------
Benchmark                                                Time             CPU   Iterations
------------------------------------------------------------------------------------------
BM_NarrowPhaseDetection_getDetectionOutputs/8         18.4 us         18.4 us        36921
BM_NarrowPhaseDetection_getDetectionOutputs/16        54.1 us         54.1 us        12873
BM_NarrowPhaseDetection_getDetectionOutputs/32         177 us          177 us         3973
BM_NarrowPhaseDetection_getDetectionOutputs/64         631 us          631 us         1116
BM_NarrowPhaseDetection_getDetectionOutputs/128       2587 us         2587 us          268
BM_NarrowPhaseDetection_getDetectionOutputs/256      10649 us        10649 us           67

sofa::helper::map_ptr_stable_compare

------------------------------------------------------------------------------------------
Benchmark                                                Time             CPU   Iterations
------------------------------------------------------------------------------------------
BM_NarrowPhaseDetection_getDetectionOutputs/8         38.3 us         38.3 us        18336
BM_NarrowPhaseDetection_getDetectionOutputs/16         234 us          234 us         3010
BM_NarrowPhaseDetection_getDetectionOutputs/32        1373 us         1373 us          506
BM_NarrowPhaseDetection_getDetectionOutputs/64        8055 us         8056 us           88
BM_NarrowPhaseDetection_getDetectionOutputs/128      45349 us        45348 us           15
BM_NarrowPhaseDetection_getDetectionOutputs/256     243190 us       243189 us            3

The benchmarks were performed using https://github.com/alxbilger/SofaBenchmark.
BM_NarrowPhaseDetection_getDetectionOutputs/n corresponds to a map containing n^2 pairs of CollisionModel*.

Discussion

It is undeniable that std::map and std::unordered_map are faster than sofa::helper::map_ptr_stable_compare. However, I don't know how crucial it is to have this kind of map with a dedicated comparator. Is this feature necessary? If not, I am in favour of using std::unordered_map wherever sofa::helper::map_ptr_stable_compare is used, for performances reasons.

Overall, we won't get a huge gain of fps, but still it has an impact on the peformances of NarrowPhaseDetection. Initially, I wondered why NarrowPhaseDetection took so much time for so simple instructions.

---

By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).

---

Reviewers will merge this pull-request only if

• it builds with SUCCESS for all platforms on the CI.
• it does not generate new warnings.
• it does not generate new unit test failures.
• it does not generate new scene test failures.
• it does not break API compatibility.
• it is more than 1 week old (or has fast-merge label).

[alxbilger]

[ci-build][with-all-tests]

[alxbilger]

See #2053 for real-life consequences of the choice of the map.

[fredroy]

I am not sure of the consequence to change the maps, but if :

• the tests (yours + current scene tests) behave exactly the same,
• it is easy to revert in case we find drawbacks later on

I am ok with that.

[jnbrunet]

Looks good to me. I looked quickly at the "delete from within a loop" thing, and I can't see where it could fail. So 👍 for this change on my side.