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

[MLA-1909] Match3 and Camera/RenderTexture sensor GC improvements #5233

Merged
merged 9 commits into from
Apr 12, 2021

Conversation

chriselion
Copy link
Contributor

@chriselion chriselion commented Apr 8, 2021

Proposed change(s)

  • Reuse Texture2Ds to avoid allocations
  • Agent now calls Dispose() on any ISensors that implement IDisposable.
  • Special-case RGB24 texture->ObservationWriter (which is what Camera and RenderTexture Sensors user for inference) to avoid GetPixels32(), which allocates a copy of the pixel data.

Match3Sensor

Before
image
After
image

GridWorld (Camera + RenderTexture Sensors)

Before
image
After
image

Useful links (Github issues, JIRA tickets, ML-Agents forum threads etc.)

https://jira.unity3d.com/browse/MLA-1909
https://jira.unity3d.com/browse/MLA-1910
https://jira.unity3d.com/browse/MLA-208

Types of change(s)

  • Bug fix
  • New feature
  • Code refactor

Checklist

  • Added tests that prove my fix is effective or that my feature works
  • Updated the changelog (if applicable)

Other comments

for (var i = 0; i < sensors.Count; i++)
{
var sensor = sensors[i];
if (sensor is IDisposable disposableSensor)
Copy link
Contributor

Choose a reason for hiding this comment

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

fannnncy

{
if (!ReferenceEquals(null, m_Sensor))
{
m_Sensor.Dispose();
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be a double dispose? looks safe since the sensor checks for null. Just curious if it's actually needed or not.

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 made sure all the sensor.Dispose's are idempotent - they check for null textures first, then set the texture to null.

Copy link
Contributor

@surfnerd surfnerd left a comment

Choose a reason for hiding this comment

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

looks good, just not sure if the extra dispose in the components are needed anymore since the sensor disposal happens in Agent.

@chriselion chriselion merged commit ea64e2b into main Apr 12, 2021
@delete-merged-branch delete-merged-branch bot deleted the MLA-1909-match3-gc branch April 12, 2021 23:28
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants