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

Publish performance metrics #557

Open
chapulina opened this issue Jan 13, 2021 · 12 comments
Open

Publish performance metrics #557

chapulina opened this issue Jan 13, 2021 · 12 comments
Assignees
Labels
🏰 citadel Ignition Citadel migration Helps with migration from Gazebo classic to Ignition performance Runtime performance

Comments

@chapulina
Copy link
Contributor

Performance metrics have been recently added to Gazebo classic, see Gazebo: Tutorial: Performance metrics.

It would be interesting to publish the same metrics from Ignition simulations, so that users can:

  • Get measurable data to see how different scenarios and parameters affect performance
  • Compare performance between Gazebo Classic and Ignition Gazebo
@chapulina chapulina added migration Helps with migration from Gazebo classic to Ignition performance Runtime performance labels Jan 13, 2021
@chapulina
Copy link
Contributor Author

For reference, here's the PR where the feature was implemented in Gazebo classic: gazebosim/gazebo-classic#2819. The new message should be added to ign-msgs.

And it's important to note that the original PR introduced a deadlock that was fixed in gazebosim/gazebo-classic#2917. Something to be aware of when implementing the same here.

This PR can target Citadel.

@chapulina chapulina added the 🏰 citadel Ignition Citadel label May 6, 2021
@francocipollone francocipollone self-assigned this Jun 30, 2021
@francocipollone
Copy link
Contributor

Hey, a couple of comments/question:

For reference this is the proto message to be published:

Click to see the proto message description
message PerformanceMetrics
{
  /// \brief This message contains information about the performance of
  /// each sensor in the world.
  /// If the sensor is a camera then it will publish the frame per second (fps).
  message PerformanceSensorMetrics
  {
    /// \brief Sensor name
    string name             = 1;

    /// \brief The update rate of the sensor in real time.
    double real_update_rate = 2;

    /// \brief The update rate of the sensor in simulation time.
    double sim_update_rate  = 3;

    /// \brief Optional fps data. If the sensor is a camera then this field should be filled
    /// with average fps in real time.
    double fps                     = 4;
  }

  /// max_step_size x real_time_update_rate sets an upper bound of
  /// real_time_factor. If real_time_factor < 1 the simulation is
  /// slower than real time.
  double real_time_factor         = 1;

  /// Each sensor in the world will create a PerformanceSensorMetrics
  /// message publishing information about the performance.
  repeated PerformanceSensorMetrics sensor = 2;
}
  1. Who should publish the information?
    In Gazebo Classic we have a SensorManager class and feels correct to publish from there given that the performance metrics are mainly based on the performance of the sensors. However, in Ignition, the ownership of the sensors isn't centralized.

    a. It seems right to let SimulationRunner be the one that publishes the PerformanceMetrics information, given that it is already publishing WorldStatistics msg at /world/<name>/stats, and besides, if another metric(non related to sensor) is needed to be added to the PerformanceMetric message probably from there we can manage it.
    Note: I was thinking that If we implement it in the SimulationRunner there is the possibility to extend the WorldStatistics msg with the PerformanceMetrics information instead of having two different topics that are being advertised with "stats"

    b. It is tempting to use the Sensor System to publish this information given that it gathers all the RenderingSensors and probably makes no sense to print information about sensors non-rendering sensors like Altimeter that has a more trivial implementation than RenderingSensors. However, for consistency, I believe that it is a message that we will want to publish even when we don't have the Sensors System loaded and for all the sensors.

  2. How accessing the ignition::sensors::Sensors instances?
    The Sensor System owns the sensors instances(through a ignition::sensors::Manager instance).
    It will depend on from where we are publishing the metrics how directly we can get the info:

    • (1a)Publishing from SimulationRunner: There is no easy way to get the Sensors (Probably I am missing something)
    • (1b)Publishing from Sensor System: Sensor system owns them so it is easy to get a pointer to the sensors and call the methods that ignition::sensors::Sensor's API provides to get the necessary information to create the metric message.
  3. Calculating the sensor metrics
    In Gazebo Classic, the Sensor's interface defines a method called LastMeasurmentTime that allows us, combined with the realTime of the simulation, to calculate the real_update_rate and the sim_update_rate for each sensor.
    In ignition we have ignition::sensors::Sensor::NextUpdateTime that returns the time of the next generation of data of the sensor, so I haven't deeply analyzed it but probably I could get the metric from there. Otherwise, I will need to extend the interface a bit.

@chapulina
Copy link
Contributor Author

These are all very good questions!

n Ignition, the ownership of the sensors isn't centralized

One idea that crossed my mind is that maybe each sensor could publish its own metrics. This logic could live in ignition::sensors::Sensors. The downside is that the information would be spread across multiple topics, instead of aggregated in a single one. But on the upside, users can easily subscribe just to the sensor metrics they're interested in and don't need to parse the message to find the sensor they care about.

Would that be easier than doing it from the SimulationRunner or a system?

instead of having two different topics that are being advertised with "stats"

Yeah I agree that this duplication is not good. For this task, I'd be ok completely dropping the real_time_factor field and just publishing the sensor metrics. Then the stats topic continues being the authority on RTF. This would be nice so we don't end up in the messy situation that Gazebo classic is in, with multiple sources of RTF.

@francocipollone
Copy link
Contributor

francocipollone commented Jul 9, 2021

One idea that crossed my mind is that maybe each sensor could publish its own metrics. This logic could live in ignition::sensors::Sensors. The downside is that the information would be spread across multiple topics, instead of aggregated in a single one. But on the upside, users can easily subscribe just to the sensor metrics they're interested in and don't need to parse the message to find the sensor they care about.

Would that be easier than doing it from the SimulationRunner or a system?

Yes that's would be easier. I am just thinking that from Sensor we don't know about simTime, so we wouldn't be able to get both the sim_update_rate metric, only real_update_rate. Am I right?

@francocipollone
Copy link
Contributor

The common::time that sensors::Sensor::Update() method is receiving corresponds to the simulation time.

https://github.com/ignitionrobotics/ign-sensors/blob/ca0dab3e513ccf24e9f60e2d4ae1ec82fcc37091/src/Sensor.cc#L243-L267

Given that this variable comes from this called(In the case of the Sensors System)

https://github.com/ignitionrobotics/ign-gazebo/blob/7cc8c095db61f45f5677372006d0e304fe4b6515/src/systems/sensors/Sensors.cc#L264

So, having said that, given that we receive the simTime we could obtain the simUpdateRate of the sensor
and in order to get the realUpdateRate we could just use std::chrono::steady_clock::now() calls to calculate the real time between updates.

This all could be implemented within sensors::Sensor

Unless I am missing something I think that we have a clear path to follow.

One last thing. What about adding also to the metrics message the information about the "nominal" update rate of the sensor?

https://github.com/ignitionrobotics/ign-sensors/blob/ca0dab3e513ccf24e9f60e2d4ae1ec82fcc37091/include/ignition/sensors/Sensor.hh#L118

@chapulina
Copy link
Contributor Author

What about adding also to the metrics message the information about the "nominal" update rate of the sensor?

I like that idea 👍

I think that we have a clear path to follow.

Great, thanks!

@francocipollone
Copy link
Contributor

I like that idea +1

😃

Some updates: I've made progress here:

The only thing left is related to the fps field of the message. This field makes sense when the sensor is a CameraSensor.

CameraSensor::RenderingCamera() method returns a pointer to rendering::Camera and from there I could call the rendering::Camera::AvgFPS method.

But RenderingCamera() isn't in the Sensor's API so from Sensor I can't get that information.

@chapulina
Copy link
Contributor Author

One idea would be to make the PublishMetrics function virtual, so sensors like cameras could implement their own function.

Another thing that stands out to me is that it would be nice to be able to disable the metrics publishing. For now we can just add the API to ign-sensors, and in the future there could be a way to set it from SDF or ign-gazebo's CLI.

@francocipollone
Copy link
Contributor

One idea would be to make the PublishMetrics function virtual, so sensors like cameras could implement their own function.

Another thing that stands out to me is that it would be nice to be able to disable the metrics publishing. For now we can just add the API to ign-sensors, and in the future there could be a way to set it from SDF or ign-gazebo's CLI.

Thanks for the suggestions! 👍

@francocipollone
Copy link
Contributor

I thought that rendering::Camera already has an AvgFPS method but I'd got confused with gazebo classic, my mistake.

ignition::rendering::Camera doesn't provide api to get the average fps from rendering.
Adding this method would imply modifying:

@chapulina If you consider it is ok I could go for it.

@chapulina
Copy link
Contributor Author

Ah I see. Since we're short on time for this task, how about we leave FPS as future work and just focus on update rate for now?

@francocipollone
Copy link
Contributor

francocipollone commented Jul 21, 2021

Ah I see. Since we're short on time for this task, how about we leave FPS as future work and just focus on update rate for now?

Great, sounds reasonable.

I leave below some next steps based on things discussed in the issue:

nkoenig pushed a commit that referenced this issue Nov 8, 2021
3 to 4 logging behavior and tests

Approved-by: John Shepherd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏰 citadel Ignition Citadel migration Helps with migration from Gazebo classic to Ignition performance Runtime performance
Projects
None yet
Development

No branches or pull requests

2 participants