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

WIP HITL multiprocessing speedup #1653

Draft
wants to merge 1 commit into
base: eundersander/SIRo_hitl_Sep5_demo_refactored
Choose a base branch
from

Conversation

eundersander
Copy link
Contributor

@eundersander eundersander commented Oct 25, 2023

Motivation and Context

MultiprocessDriverWrapper is a wrapper intended to be used with SandboxDriver (or any GuiAppDriver). It creates a separate "sim process" to run the driver, with the goal to overlap (1) sim-updating and (2) rendering in ReplayGuiAppRenderer (used by GuiApplication). The ultimate goal is runtime speedup.

Using the Oct 18 snapshot, in scene 2, on 2021 Macbook, I see the minimum FPS improve from 26 to 31. Note the minimum FPS is usually when Spot is busy, particularly when it's backing up.

This is a work in progress. Various todos are listed in the code. I'll try to summarize here:

  • Text-drawing and Line-rendering are unsolved. In the old single-threaded code, we directly share these objects between the driver and GuiApplication. In this multiprocessing paradigm, we need to utilize a MP data structure like Queue to transmit the requests for text-drawing and line-rendering from the sim process to the main process.
  • GuiInput is partially broken, in particular, zooming with mouse-scroll. I suspect I'm calling GuiInput.on_frame_end at the wrong time on the main process, causing some input events to be lost instead of sent to the sim process.
  • Better cleanup for the sim process on shutdown.
  • Decouple MultiprocessDriverWrapper and SandboxDriver: there's a comment in code about this.
  • More robust testing. We should make sure we haven't overlooked a way for sim_update to get called without a GuiInput having been provided via the queue; this would cause the app to hang, as the main process would be waiting for a post_sim_update_dict and the sim process would be waiting for a GuiInput.
  • Clean up Magnum pickling. See this Slack thread.

How Has This Been Tested

Local testing on 2021 Macbook. Note I haven't tested with --remote-gui-mode but I don't anticipate problems.

Types of changes

PR into a non-main branch.

Checklist

  • My code follows the code style of this project.
  • I have updated the documentation if required.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes if required.

@eundersander eundersander added the do not merge Not ready to merge. This label should block merging. label Oct 25, 2023
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Oct 25, 2023
Copy link
Contributor Author

@eundersander eundersander left a comment

Choose a reason for hiding this comment

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

Some comments for reviewers

# fix for unpickable type; run once at startup
copyreg.pickle(mn.Vector3, pickle_vector3)
copyreg.pickle(mn.Vector2i, pickle_vector2i)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this approach produces a very bloated serialization (every serialized Vector2i includes the unpickle_vector2i string among other junk!). @mosra is working on proper pickle support for Magnum types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not that the serialization introduced in mosra/magnum-bindings@f561337 would produce anything significantly better -- it still has to encode the actual type as a string for each such instance. But as long as the serialized data is an arbitrary object tree / a JSON-like structure instead of a small set of typed (multi-dimensional) arrays with many elements, there's no way around that.

# using OpenGL on 2+ threads in the same process. We probably just can't
# support using MultiprocessDriverWrapper with dummy multiprocessing (which
# is fine; dummy multiprocessing is more for debugging).
assert not multiprocessing_config.use_dummy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess what I'm saying is that this code should use multiprocessing.Process directly instead of multiprocessing_config. For context, I originally created multiprocessing_config to make it easy for our multithreaded networking/server code to switch between real and dummy multiprocessing.

block=True
)

sim_gui_input.shallow_copy_from(latest_gui_input)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a goofy paradigm for "syncing" an object across the process boundary (basically, sending the entire object across, then doing this shallow copy). Maybe we do something similar for TextDrawer and DebugLineRenderer. Also maybe there's a more pythonic way to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. do not merge Not ready to merge. This label should block merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants