Fast-DDS-statistics-backend icon indicating copy to clipboard operation
Fast-DDS-statistics-backend copied to clipboard

Efficiency get entity search [12001]

Open jparisu opened this issue 3 years ago • 3 comments

Features:

  • Add EntityKind to search in get_entity call more efficiently in the case the kind of the entity is known.
  • Change BUILD_TYPE to Release in default
  • Change colcon.meta in CI so it is compiled in Release and Debug (should speed up clang)

ToDo: Must discuss if the public API could be extended using the EntityKind in several methods to speed up the search in entity database.

jparisu avatar Jul 08 '21 06:07 jparisu

Codecov Report

Merging #110 (2140683) into main (3b4a364) will increase coverage by 0.65%. The diff coverage is 84.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #110      +/-   ##
==========================================
+ Coverage   58.89%   59.55%   +0.65%     
==========================================
  Files          31       31              
  Lines        4107     4117      +10     
  Branches     2169     2160       -9     
==========================================
+ Hits         2419     2452      +33     
- Misses         54       55       +1     
+ Partials     1634     1610      -24     
Impacted Files Coverage Δ
src/cpp/database/database.hpp 79.36% <ø> (ø)
src/cpp/database/database_queue.cpp 54.94% <40.00%> (+0.24%) :arrow_up:
...c/cpp/subscriber/StatisticsParticipantListener.cpp 59.51% <64.28%> (+1.22%) :arrow_up:
src/cpp/database/database.cpp 53.11% <92.18%> (+1.16%) :arrow_up:
src/cpp/database/database_queue.hpp 65.71% <0.00%> (+0.71%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3b4a364...2140683. Read the comment docs.

codecov[bot] avatar Jul 13 '21 14:07 codecov[bot]

It would be interesting to analyze why the partial coverage lines are fully covered for the PARTICIPANT, DATAREADER, DATAWRITER and LOCATOR cases, and not the others. That could give us some insight about what kind of tests we may need (instead of creating a dummy test only for the sake of getting a full coverage). The case for the LOCATOR is specially interesting. All the others are entities that we discover directly through PDP/EDP, but I would expect the LOCATOR and the TOPIC to have the same behavior if that were the only difference.

The second point is more clear to me. If the current logic does not allow the condition, then we can remove it altogether or convert it to an assert. Otherwise, we need a test (probably with a mock, as suggested) in order to check the correct behavior.

IkerLuengo avatar Jul 14 '21 07:07 IkerLuengo

There have been several changes. Now the new efficient method has been added to the listeners and so the tests has changed in EXPECTED_CALL.

Regarding coverage fails:

  1. It was not called because the listeners did not use them yet, so only few calls used it.
  2. Now it should be fixed. Cases that could not happen has been changed by asserts.

jparisu avatar Jul 16 '21 08:07 jparisu