[Gazebo 11] Added ignition common profiler #2776

ahcorde · 2020-07-03T16:28:17Z

I added profile points in ODEPhysics, sensors and some plugins

Here some snapshots:

there is a world called profile.world where you can reproduce these results.

I had to change this line in the CMakeLists.txt to avoid create a Profile Singleton in each library. I can provide more details about this if necessary.

from

  filter_valid_compiler_flags(-fvisibility=hidden -fvisibility-inlines-hidden)

to

  filter_valid_compiler_flags(-fvisibility-inlines-hidden)

is this change creating a conflict with something specific?

Signed-off-by: ahcorde [email protected]

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

iche033

works for me. I have some minor comments about style and profile names.

iche033 · 2020-07-11T02:24:39Z

CMakeLists.txt

@@ -260,7 +268,7 @@ filter_valid_compiler_flags(${WARN_LEVEL}
 # Check and add visibility hidden by default. Only in UNIX
 # Windows and MacosX does not handled properly the hidden compilation
 if (UNIX AND NOT APPLE)
-  filter_valid_compiler_flags(-fvisibility=hidden -fvisibility-inlines-hidden)
+  filter_valid_compiler_flags(-fvisibility-inlines-hidden)


maybe @j-rivero can comment on whether this change would be a problem or not?

friendly ping @j-rivero

uh, the change is important. Symbols are exposed by default in the ABI unless the fvisibility=hidden flag is provided. This change will increase the ABI size (with an impact on the loader performance) and expose many new symbols to the binary interface. Can we make it conditional somehow to keep the current ABI scheme?

Added a9230be

gazebo/Server.cc

gazebo/sensors/Sensor.cc

gazebo/sensors/SensorManager.cc

gazebo/physics/ode/ODEPhysics.cc

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

iche033 · 2020-07-14T19:51:39Z

latest changes look good to me. Just waiting to see if @j-rivero has any thoughts on whether or not it's ok to remove the -fvisibility=hidden compiler flag

scpeters · 2020-07-15T23:06:29Z

gazebo/Server.cc

      sensors::stop();
+      IGN_PROFILE_END();
+    }

    common::Time::MSleep(1);


hmm...having a 1 ms sleep in the server runloop seems non-ideal

I remember seeing this before and wondering why it's there 🧐

I saw this while working on lockstepping. I tried removing the line but it caused some sensor tests to fail so I had to put it back in:
https://github.com/osrf/gazebo/pull/2746/files#diff-2e1da9464c22259ef9d9919412c42d2fR617

I wonder if we can we change it to NSleep(1)?

this should happen in a separate PR; I'll try creating a PR with this change

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

…ign_common

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

iche033 · 2020-07-27T19:35:46Z

latest changes look good. There are a few new code check errors found by running sh tools/code_check.sh

ahcorde · 2020-07-28T16:46:41Z

@iche033 Any trick to show only the new ones?

iche033 · 2020-07-28T18:12:20Z

oh I don't know a trick to show just the new errors. I just manually ran code check and checked the errors against the code. Here're the ones I see:

./plugins/LiftDragPlugin.cc:404:  Blank line at the end of a code block.  Is this needed?  [whitespace/blank_line] [3]
./plugins/LinkPlot3DPlugin.cc:207:  { should never be at the end of the previous line  [whitespace/braces] [4]
./gazebo/sensors/WideAngleCameraSensor.cc:185:  { should never be at the end of the previous line  [whitespace/braces] [4]
./gazebo/sensors/SensorManager.cc:19:  Found C++ system header after other header. Should be: c system, c++ system, other, SensorManager.h .  [build/include_order] [4]
./gazebo/sensors/SensorManager.cc:20:  Found C++ system header after other header. Should be: c system, c++ system, other, SensorManager.h .  [build/include_order] [4]
./gazebo/physics/World.cc:40:  Found C++ system header after other header. Should be: c system, c++ system, other, World.h .  [build/include_order] [4]
./gazebo/common/Event.hh:32:  Found C++ system header after other header. Should be: c system, c++ system, other, Event.h .  [build/include_order] [4]
./gazebo/common/Event.hh:280:  { should never be at the end of the previous line  [whitespace/braces] [4]
./gazebo/common/Event.hh:300:  { should never be at the end of the previous line  [whitespace/braces] [4]
./gazebo/common/Event.hh:321:  { should never be at the end of the previous line  [whitespace/braces] [4]
./gazebo/common/Event.hh:343:  { should never be at the end of the previous line  [whitespace/braces] [4]
./gazebo/common/Event.hh:367:  { should never be at the end of the previous line  [whitespace/braces] [4]
./gazebo/common/Event.hh:393:  { should never be at the end of the previous line  [whitespace/braces] [4]
./gazebo/common/Event.hh:420:  { should never be at the end of the previous line  [whitespace/braces] [4]
./gazebo/common/Event.hh:448:  { should never be at the end of the previous line  [whitespace/braces] [4]

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

* 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]>

Added profiler to OdePhysics, sensors and some plugins

7e5681b

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

ahcorde mentioned this pull request Jul 6, 2020

Integrate ign-common's profiler #2720

Closed

ahcorde added 3 commits July 6, 2020 12:31

Added profiler to Physic engines: Bullet, DART and Simbody

04e3a92

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

Added profiler to plugins

96c30fc

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

Added more models to profiler.world

4a89c15

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

ahcorde changed the title ~~Added profiler to OdePhysics, sensors and some plugins~~ Added ignition comoon profiler to Gazebo 11 Jul 6, 2020

Add missing ;

dbae76f

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

ahcorde marked this pull request as ready for review July 7, 2020 19:04

scpeters changed the title ~~Added ignition comoon profiler to Gazebo 11~~ Added ignition common profiler to Gazebo 11 Jul 9, 2020

ahcorde mentioned this pull request Jul 9, 2020

[Gazebo 9] Backport Added ignition common profiler #2783

Closed

ahcorde changed the title ~~Added ignition common profiler to Gazebo 11~~ [Gazebo 11] Added ignition common profiler Jul 10, 2020

ahcorde mentioned this pull request Jul 10, 2020

Added profiler tutorial osrf/gazebo_tutorials#112

Merged

Added option ENABLE_PROFILER to cmake

265c0c9

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

iche033 reviewed Jul 11, 2020

View reviewed changes

Used the same convention in all profiler calls

f8d068f

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

scpeters added the 11 Gazebo 11 label Jul 15, 2020

scpeters reviewed Jul 15, 2020

View reviewed changes

ahcorde requested review from scpeters, iche033 and chapulina July 21, 2020 10:12

Added profiler to Event.hh

75a49f4

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

ahcorde mentioned this pull request Jul 23, 2020

[Noetic] Added Ignition common profiler to gazebo_plugins ros-simulation/gazebo_ros_pkgs#1139

Merged

ahcorde self-assigned this Jul 23, 2020

ahcorde added 3 commits July 24, 2020 15:41

Merge remote-tracking branch 'origin/gazebo11' into ahcorde/profiler/…

2efc894

…ign_common

Fixed gazebo_common link against ignition_common

ae9a89d

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

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

a9230be

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

make linters happy

a63a293

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

iche033 approved these changes Jul 29, 2020

View reviewed changes

ahcorde merged commit 3e5ffc9 into gazebosim:gazebo11 Jul 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Gazebo 11] Added ignition common profiler #2776

[Gazebo 11] Added ignition common profiler #2776

ahcorde commented Jul 3, 2020 •

edited

Loading

iche033 left a comment

iche033 Jul 11, 2020

ahcorde Jul 21, 2020

j-rivero Jul 27, 2020

ahcorde Jul 27, 2020

iche033 commented Jul 14, 2020

scpeters Jul 15, 2020

chapulina Jul 16, 2020

iche033 Jul 16, 2020

scpeters Jul 17, 2020

iche033 commented Jul 27, 2020

ahcorde commented Jul 28, 2020

iche033 commented Jul 28, 2020 •

edited

Loading

[Gazebo 11] Added ignition common profiler #2776

[Gazebo 11] Added ignition common profiler #2776

Conversation

ahcorde commented Jul 3, 2020 • edited Loading

iche033 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iche033 commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iche033 commented Jul 27, 2020

ahcorde commented Jul 28, 2020

iche033 commented Jul 28, 2020 • edited Loading

ahcorde commented Jul 3, 2020 •

edited

Loading

iche033 commented Jul 28, 2020 •

edited

Loading