Use ComponentState::PeriodicChange in UpdateState to avoid forcing full scene update

[ivanpauno]

When trying to implement primary-secondary async stepping in #481, I found out that there was almost no speed-up in doing that because the primary was running quite slowly.

After some profiling I realized that the SceneBroadcaster::PostUpdate step was much slower than in an standalone simulation because a full scene update was always forced.

This PR modifies EntityComponentManager::SetState(const ignition::msgs::SerializedStateMap &_stateMsg) so when a component is updated a ComponentState::PeriodicChange change is set instead of a ComponentState::OneTimeChange, avoiding to regenerate a full scene all the time.

With this change, the distributed_levels example works ~4 times faster when using a distributed simulation.

I'm not sure if using ComponentState::PeriodicChange is always correct, so we can consider the following alternatives:

• NetworkManagerPrimary::Step is not the only consumer of EntityComponentManager::SetState.
  If for the other consumers doing a ComponentState::OneTimeChange is not a problem and for NetworkManagerPrimary::Step using ComponentState::PeriodicChange is always ok, I could add an extra argument to the method.
• If for some components updates ComponentState::OneTimeChange make more sense and for others ComponentState::PeriodicChange make more sense, the we should add that information to the SerializedStateMap message.

[mjcarroll]

With this change, the distributed_levels example works ~4 times faster when using a distributed simulation.

Four times faster than a standalone sim, or the previous implementation?

[ivanpauno]

Four times faster than a standalone sim, or the previous implementation?

Than the previous implementation.

I don't think that the simple distributed_levels example can ever run faster in "distributed mode" than in "standalone mode", because the world is extremely simple.

[ivanpauno]

Some actual numbers in my machine using the distributed_levels world

• Current master, standalone: RTF ~ 1000%
• Current master, distributed: RTF ~ 40%
• This PR, distributed: RTF ~ 150%

Using the distributed_levels world but replacing the sphere and the box with two EXPLORER_X1_SENSOR_CONFIG_2.

• Current master, standalone: RTF ~ 150%
• Current master, distributed: RTF ~ 18%
• This PR, distributed: RTF ~ 70%
• async stepping without this change, distributed: RTF ~ 20%
• async stepping with this change, distributed: RTF ~ 120%

Note: The RTF numbers above is an "average" I made from visually inspecting the RTF factor in the GUI, so don't get them too seriously ... it's just an rough approximation to understand the performance benefit.

[nkoenig]

Could this be changed to target citadel --> ign-gazebo3?

[ivanpauno]

Could this be changed to target citadel --> ign-gazebo3?

I took a quick look and targeting ign-gazebo3 branch doesn't look hard.
I will update the target branch tomorrow.

(edit) tomorrow, not on Monday 😂

[ivanpauno]

@nkoenig do you mean only this PR or also #481?

[ivanpauno]

Updated base branch to ign-gazebo3, I can open another one targeting ign-gazebo4 if desired.
@nkoenig let me know if you want a version of #481 targeting ign-gazebo3 too.

[codecov]

Codecov Report

Merging #486 (6c57ab2) into main (d392223) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #486      +/-   ##
==========================================
+ Coverage   77.56%   77.62%   +0.05%     
==========================================
  Files         211      211              
  Lines       11579    11582       +3     
==========================================
+ Hits         8981     8990       +9     
+ Misses       2598     2592       -6     

Impacted Files	Coverage Δ
src/EntityComponentManager.cc	84.52% <100.00%> (+0.02%)	⬆️
src/network/NetworkManagerSecondary.cc	81.25% <100.00%> (+0.60%)	⬆️
src/SimulationRunner.cc	94.76% <0.00%> (+1.16%)	⬆️

---

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 42e4dc4...6c57ab2. Read the comment docs.

[ivanpauno]

@mjcarroll @chapulina friendly ping.
I'm not sure if the approach of this PR is correct or not, so it would be great to have feedback to know if we want to explore some of the alternatives mentioned in the PR description or if this is okay.

[chapulina]

Nice catch. My concern is that important one-time changes may be missed if we make all changes period. For example, the color of a light is changed in one secondary, and another secondary misses it and keeps seeing the old color.

we should add that information to the SerializedStateMap message.

Yeah I think this could work. We'd have to target this PR at Edifice though (main), because changing a message breaks ABI.

[ivanpauno]

Yeah I think this could work. We'd have to target this PR at Edifice though (main), because changing a message breaks ABI.

Done.
I only added one is_periodic_change field to the whole message, I could've added one field per component but I don't see the benefit of doing that.

Each time I change the base branch I bring more reviewers 😂

[chapulina]

I only added one is_periodic_change field to the whole message

Is there a PR to ign-msgs too?

I could've added one field per component but I don't see the benefit of doing that.

This is related to something @luca-della-vedova was looking into recently. They have a use case using animated actors which could take advantage of more granular OneTimeChanges, so that the state is only updated for the entities / components that had those changes. In very large worlds, it can be quite expensive to update thousands of components because one of them changed. So I think there could be value in having that field at the component level, but that may bring its own overhead.

[ivanpauno]

Is there a PR to ign-msgs too?

Now there is 😄 gazebosim/gz-msgs#119 .

This is related to something @luca-della-vedova was looking into recently. They have a use case using animated actors which could take advantage of more granular OneTimeChanges, so that the state is only updated for the entities / components that had those changes

But in that case, the changes that aren't needed can be omitted from the state message, right?
I'm not sure if more granularity is useful in the message itself ...

[chapulina]

But in that case, the changes that aren't needed can be omitted from the state message, right?

Yeah good point, they shouldn't be there at all when they aren't needed. I'm just trying to think now of any downsides to marking periodic changes as one time changes. The receiving end of that state message won't be able to tell apart which are the one time changes. Are you concerned about the overhead of marking every single component?

In any case, I think it's worth it trying the current approach. We can always add more granularity later if needed.

[chapulina]

Now that this requires ign-msgs's main branch, this is needed before it can be merged: gazebo-tooling/release-tools#362

[ivanpauno]

The receiving end of that state message won't be able to tell apart which are the one time changes

Yes that's true, if there's a potential benefit of having that information in the message maybe it's better to add it now.
AFAIS there's no code currently telling apart periodic changes from one time changes, the only thing that seems to be used is HasOneTimeComponentChanges() (there's no code calling ComponentState()).

Are you concerned about the overhead of marking every single component?

It will add some overhead, but probably minimal. It requires N lookups to this std::map while iterating the components to add to the message.

[chapulina]

there's no code currently telling apart periodic changes from one time changes

So far 😉 But we may be changing this soon for performance. In any case, I think this may not affect the internals of the State / SetState calls.

---

This looks ok for me, but we need to wait for the transition to ign-msgs7 to be complete before merging (gazebo-tooling/release-tools#362).

[nkoenig]

Updated base branch to ign-gazebo3, I can open another one targeting ign-gazebo4 if desired.
@nkoenig let me know if you want a version of #481 targeting ign-gazebo3 too.

Sorry for the late reply, I meant both.

[ivanpauno]

Sorry for the late reply, I meant both.

I have pushed a branch with #481 rebased for citadel
https://github.com/ignitionrobotics/ign-gazebo/tree/ivanpauno/secondary-async-stepping-and-primary-acknowledges-citadel and similar for the ign-imgui tool https://github.com/ivanpauno/ign_imgui/tree/citadel.
I'm still testing that though, I will comment again after checking it still works.

The instructions to build, run the simulation and see the results are here, feel free to ask if that's not clear.

PS: #481 already includes an optimization similar to this PR, so this one doesn't need to be rebased to ign-gazebo3 (and the approach taken here depeneds on gazebosim/gz-msgs#119).

[ivanpauno]

I'm still testing that though, I will comment again after checking it still works.

Tested, the citadel branches are working too.

[chapulina]

re: citadel: This needs to target Edifice because of the new message field.

[chapulina]

Let's get this in! We still have a couple months until the Edifice release to revert in case we notice something weird.