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.

Sign up for GitHub

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

Jump to bottom

Gazebo 9 profiler - take 2 #2813

Merged
merged 18 commits into from
Aug 11, 2020
Merged

Gazebo 9 profiler - take 2 #2813

merged 18 commits into from
Aug 11, 2020

Conversation

chapulina
Copy link
Contributor

This builds on top of #2783 and includes the changes I suggested on that PR:

  • Move the profiler to gazebo::common (which is analogous to ignition::common)
  • Rename all API to GZ_ so there can't be a collision with Ignition
  • Also installed gz_remotery_vis

I'm targeting gazebo9 because I think the diff becomes clearer this way.

ahcorde and others added 12 commits July 29, 2020 08:18
@ahcorde
[Gazebo 11] Added ignition common profiler (#2776)
c071fd3 
* Added profiler to OdePhysics, sensors and some plugins

Signed-off-by: ahcorde <ahcorde@gmail.com>

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

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Added profiler to plugins

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Added more models to profiler.world

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Add missing ;

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Added option ENABLE_PROFILER to cmake

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Used the same convention in all profiler calls

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Added profiler to Event.hh

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Fixed gazebo_common link against ignition_common

Signed-off-by: ahcorde <ahcorde@gmail.com>

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

Signed-off-by: ahcorde <ahcorde@gmail.com>

* make linters happy

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Profiler is a independent library gazebo_profiler
1a9aa7d 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Install gazebo_profiler when ENABLE_PROFILER is active
b70e37e 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Moved profiler from util to a new folder called profiler and try to f…
647c651 
…ix windows build

Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Merge branch 'gazebo9' into ahcorde/profiler_remotery_gazebo9
253ddae
@ahcorde
Small fix on windows
c4d3744 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Removed RMT_API from RemoteryProfilerImpl
01d35f9 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Installing one missing header
2bfb786 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina
Move profiler to common
d695df9 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
IGN -> GZ
a5696d7 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
tweaks
ab093ab 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
install visualizer
e5377c3 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina added enhancement New feature or request 9 Gazebo 9 labels Aug 5, 2020
@chapulina chapulina requested a review from iche033 August 5, 2020 00:14
iche033
iche033 reviewed Aug 5, 2020
View reviewed changes
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

works for me, just a few minor comments. One thing I noticed is that if I don't specify -DENABLE_PROFILER, I get a couple of linker errors:

libgazebo.so.9.13.2: undefined reference to `gazebo::common::Profiler::~Profiler()'
libgazebo.so.9.13.2: undefined reference to `gazebo::common::Profiler::Profiler()'

CMakeLists.txt Outdated Show resolved Hide resolved
gazebo/common/Profiler.hh Show resolved Hide resolved
gazebo/common/ProfilerImpl.hh Show resolved Hide resolved
gazebo/common/RemoteryProfilerImpl.cc Show resolved Hide resolved
gazebo/common/RemoteryProfilerImpl.hh Show resolved Hide resolved
@chapulina
always compile profiler
0afd239 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

I get a couple of linker errors:

Fixed the linker errors on 0afd239. The profiler should always be compiled, the difference is whether the macros are actual functions or just void.

@iche033
Copy link
Contributor

iche033 commented Aug 6, 2020

latest changes look good to me. The gpu-nvidia build failed due to internal compiler error so likely a false positive but the abi-check build has an error about compiling header. Maybe worth trying a rebuild

@iche033
Copy link
Contributor

iche033 commented Aug 6, 2020

@osrf-jenkins run tests please

@chapulina
Copy link
Contributor Author

Since we're not installing Profiler.hh, it can't be used by the plugins. I'll remove it from there for now.

@iche033
Copy link
Contributor

iche033 commented Aug 6, 2020

The Examples_build test failed. Looks like we also need to remove the header and macros from the examples.

@chapulina
Remove Profiler from installed header
1b298fc 
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

Argh, I had to remove the profiler from Event.hh because that's installed. No profiling for templates 😕 1b298fc

@iche033
Copy link
Contributor

iche033 commented Aug 11, 2020

windows build passed :)

@chapulina chapulina merged commit be44390 into gazebo9 Aug 11, 2020
@chapulina chapulina deleted the chapulina/ahcorde/profiler_remotery_gazebo9 branch August 11, 2020 21:12
@chapulina chapulina mentioned this pull request Aug 11, 2020
Closed
@chapulina chapulina mentioned this pull request Sep 25, 2020
Merged
@chapulina chapulina mentioned this pull request Oct 8, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iche033 iche033 iche033 approved these changes

Assignees
No one assigned
Labels
9 Gazebo 9 enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chapulina @iche033 @ahcorde