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

Add a cache to ObjectStateMachine. #2079

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add a cache to ObjectStateMachine. #2079

wants to merge 3 commits into from

Conversation

0mdc
Copy link
Contributor

@0mdc 0mdc commented Sep 15, 2024

Motivation and Context

This adds a cache to ObjectStateMachine so that get_snapshot_dict() doesn't re-compute the same snapshot over and over.

On a reference single-learn HITL application, this improves the framerate from 11.3 to 12.3 FPS.

How Has This Been Tested

Tested locally + unit tests.

Types of changes

  • [Optimization]

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.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Sep 15, 2024
Copy link
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

As it stands, this isn't going to catch cases where the states are updated externally to the state machine, such as action updates. I don't know that caching is the best solution unless the ObjectStateSpec singletons also map back to the parent ObjectStateMachine in order modify the dirty flag themselves. Even then, most of the updates use set_state_of_obj directly which doesn't hit either singleton class. For example:

None of these will be caught and the update() function is called in the simulation loop, so you're really only catching that "step" has been called since last query I think.

Copy link
Contributor Author

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

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

Added some comments for reviewers.

if osm is not None:
return osm.get_snapshot_dict(sim)
else:
return {}
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 cache is no longer necessary.

new_state = not cur_state
set_state_of_obj(obj, self.name, new_state)
return new_state

Copy link
Contributor Author

@0mdc 0mdc Sep 17, 2024

Choose a reason for hiding this comment

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

@aclegg3 : This code is unused. Let me know if you have plans for it.

It's probably not the right abstraction because toggling may have different conditions depending on the current state, e.g. Clean -> Dirty, Empty -> Full, etc.

"""

user_attr = obj.user_attributes
obj_state_config = user_attr.get_subconfig("object_states")
obj_state_config.set(state_name, state_val)
user_attr.save_subconfig("object_states", obj_state_config)

if object_state_machine is not None:
object_state_machine.set_snapshot_dirty()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aclegg3 : This is not ideal, but because set_state_of_obj() is used everywhere to change states, it does the job.

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants