Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Efficiency get entity search [12001] #110

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

jparisu
Copy link
Contributor

@jparisu jparisu commented Jul 8, 2021

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 jparisu requested a review from IkerLuengo July 8, 2021 06:20
@jparisu jparisu temporarily deployed to codecov July 8, 2021 06:20 Inactive
@jparisu jparisu temporarily deployed to codecov July 8, 2021 06:20 Inactive
@jparisu jparisu changed the title Efficiency get entity search Efficiency get entity search [12001] Jul 8, 2021
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small things mostly.

I do not know why the coverage job has not been run. It would be interesting to analyze its results.

src/cpp/database/database.cpp Outdated Show resolved Hide resolved
src/cpp/database/database.cpp Outdated Show resolved Hide resolved
src/cpp/database/database.cpp Outdated Show resolved Hide resolved
src/cpp/database/database.cpp Outdated Show resolved Hide resolved
src/cpp/database/database.cpp Outdated Show resolved Hide resolved
src/cpp/database/database.cpp Outdated Show resolved Hide resolved
src/cpp/database/database.cpp Outdated Show resolved Hide resolved
src/cpp/database/database.hpp Outdated Show resolved Hide resolved
test/unittest/Database/DatabaseTests.cpp Outdated Show resolved Hide resolved
@jparisu jparisu temporarily deployed to codecov July 12, 2021 14:58 Inactive
@jparisu jparisu force-pushed the feature/efficiency-get-entity branch from d435910 to 6843bf5 Compare July 13, 2021 05:54
@jparisu jparisu temporarily deployed to codecov July 13, 2021 05:54 Inactive
@jparisu jparisu temporarily deployed to codecov July 13, 2021 05:54 Inactive
@@ -15,7 +15,7 @@
"-DBUILD_DOCUMENTATION=ON",
"-DBUILD_TESTS=ON",
"-DCMAKE_BUILD_TYPE=Debug",
"-DCMAKE_EXPORT_COMPILE_COMMANDS=ON",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that removing this line is preventing the coverage job to run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about it.
Added in commit 302cb09 to check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us see if it works now

src/cpp/database/database.cpp Outdated Show resolved Hide resolved
@jparisu jparisu temporarily deployed to codecov July 13, 2021 07:55 Inactive
@jparisu jparisu temporarily deployed to codecov July 13, 2021 07:55 Inactive
@jparisu jparisu force-pushed the feature/efficiency-get-entity branch from 302cb09 to ef43f17 Compare July 13, 2021 13:26
@jparisu jparisu temporarily deployed to codecov July 13, 2021 13:26 Inactive
@jparisu jparisu temporarily deployed to codecov July 13, 2021 13:26 Inactive
@jparisu jparisu temporarily deployed to codecov July 13, 2021 14:11 Inactive
@jparisu jparisu temporarily deployed to codecov July 13, 2021 14:11 Inactive
@codecov
Copy link

codecov bot commented Jul 13, 2021

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%) ⬆️
...c/cpp/subscriber/StatisticsParticipantListener.cpp 59.51% <64.28%> (+1.22%) ⬆️
src/cpp/database/database.cpp 53.11% <92.18%> (+1.16%) ⬆️
src/cpp/database/database_queue.hpp 65.71% <0.00%> (+0.71%) ⬆️

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.

Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverting the changes to the CI have correctly generated the coverage report again. Looking into it I want to bring attention into these lines:

@IkerLuengo
Copy link
Contributor

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.

Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
…nd Debug mode

Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
Signed-off-by: jparisu <javierparis@eprosima.com>
@jparisu
Copy link
Contributor Author

jparisu commented Jul 16, 2021

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.

Signed-off-by: jparisu <javierparis@eprosima.com>
@jparisu jparisu temporarily deployed to codecov July 16, 2021 09:33 Inactive
@jparisu jparisu temporarily deployed to codecov July 16, 2021 09:33 Inactive
Copy link
Contributor

@JLBuenoLopez JLBuenoLopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM though it needs to be rebased

@EduPonz EduPonz modified the milestone: v0.4.0 Dec 10, 2021
@JLBuenoLopez JLBuenoLopez added this to the v0.8.2 milestone Mar 16, 2023
@JLBuenoLopez JLBuenoLopez modified the milestones: v0.8.2, v0.9.1 Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants