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

[Gazebo 9] Backport Added ignition common profiler #2783

Closed

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Jul 9, 2020

This PR is a backport of this other PR #2776

Gazebo 9 uses ign-common1, in this version of ignition common the profiler is not implemented yet. To backport this functionality into ign-common requires to also backport components into ign-cmake0. I have talked with @chapulina and it makes sense to include the profiler code inside the Gazebo code. In particular I have added this code in gazebo utils where the other profiler ( Diagnostic) it's also located.

ahcorde and others added 2 commits July 29, 2020 08:18
* Added profiler to OdePhysics, sensors and some plugins

Signed-off-by: ahcorde <[email protected]>

* Added profiler to Physic engines: Bullet, DART and Simbody

Signed-off-by: ahcorde <[email protected]>

* Added profiler to plugins

Signed-off-by: ahcorde <[email protected]>

* Added more models to profiler.world

Signed-off-by: ahcorde <[email protected]>

* Add missing ;

Signed-off-by: ahcorde <[email protected]>

* Added option ENABLE_PROFILER to cmake

Signed-off-by: ahcorde <[email protected]>

* Used the same convention in all profiler calls

Signed-off-by: ahcorde <[email protected]>

* Added profiler to Event.hh

Signed-off-by: ahcorde <[email protected]>

* Fixed gazebo_common link against ignition_common

Signed-off-by: ahcorde <[email protected]>

* Added condition to use -fvisibility=hidden when profiler is disabled

Signed-off-by: ahcorde <[email protected]>

* make linters happy

Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde force-pushed the ahcorde/profiler_remotery_gazebo9 branch from 8605a66 to 1a9aa7d Compare July 29, 2020 07:24
@ahcorde
Copy link
Contributor Author

ahcorde commented Jul 29, 2020

We should "merge" this PR if we want to keep the history with the Gazebo 11 branch instead of "squash and merge".

@iche033
Copy link
Contributor

iche033 commented Jul 29, 2020

jenkins build caught a cmake error and I was able to reproduce it locally. If -DENABLE_PROFILER=1 is not specified when running cmake, I get this:

CMake Error at cmake/GazeboUtils.cmake:133 (set_target_properties):
  set_target_properties Can not find target to add properties to:
  gazebo_profiler
Call Stack (most recent call first):
  gazebo/util/CMakeLists.txt:179 (gz_install_library)

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

I have added this code in gazebo utils

It looks like you created a new library, gazebo_profiler, instead of adding it to gazebo_utils. Is there a reason for that? Creating a new library comes with extra overhead.


The examples are failing to build, see the results for the EXAMPLE_examples_build test:

375: In file included from /usr/include/gazebo-9/gazebo/common/common.hh:18:0,
375:                  from /usr/include/gazebo-9/gazebo/gazebo_client.hh:23,
375:                  from /var/lib/jenkins/workspace/gazebo-ci-pr_any-ubuntu_auto-amd64-gpu-none/gazebo/examples/stand_alone/transporter/transporter.cc:18:
375: /usr/include/gazebo-9/gazebo/common/Event.hh:32:39: fatal error: gazebo/profiler/Profiler.hh: No such file or directory

We need to make sure that it's possible to compile against Gazebo even if the profiler is not enabled.


gz_install_library(gazebo_profiler)

gz_install_includes("profiler" Profiler.hh)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not install this header? If we do, we'll need to go through a tick-tock deprecation cycle. I think for Gazebo <= 9 we can keep the profiler internal, and for Gazebo 11 + we expose the ign-common profiler as we're doing for gazebo_ros_pkgs.

@@ -247,6 +254,7 @@ target_compile_definitions(gazebo_common

target_link_libraries(gazebo_common
${IGNITION-MATH_LIBRARIES}
${IGNITION-COMMON_LIBRARIES}
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 this can be removed.

*/

#ifndef IGNITION_COMMON_PROFILER_HH_
#define IGNITION_COMMON_PROFILER_HH_
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#define IGNITION_COMMON_PROFILER_HH_
#define GAZEBO_COMMON_PROFILER_HH_

There are other instances too

}
}

#ifndef IGN_PROFILER_ENABLE
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change all of these from IGN to GZ? Just to be safe in case the 2 of them ever get mixed in the same project.

@@ -0,0 +1,1390 @@
<?xml version="1.0" ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you comment on what this world is supposed to be demonstrating?

@chapulina
Copy link
Contributor

This was superceded by #2813, which includes this PR.

@chapulina chapulina closed this Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9 Gazebo 9
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants