Skip to content

Conversation

chriselion
Copy link
Contributor

@chriselion chriselion commented Aug 27, 2020

Proposed change(s)

Follow-up optimization attempt from #4399

This was basically a wash performance-wise; we can keep if for cleanliness, or just drop the whole idea.

Tested with
mlagents-learn config/ppo/GridWorld.yaml --env=Project/Build/grid --force --num-envs=4
using 1/10 max steps

before
2020-08-26 20:29:32 INFO [stats.py:118] GridWorld. Step: 50000. Time Elapsed: 132.532 s. Mean Reward: -0.195. Std of Reward: 0.952. Training.

"process_pixels": {
    "total": 21.524973707002246,
    "count": 45987,
    "is_parallel": true,
    "self": 4.624946935002843,
    "children": {
        "image_decompress": {
            "total": 16.900026771999404,
            "count": 45987,
            "is_parallel": true,
            "self": 16.900026771999404
        }
    }
}


after
2020-08-26 20:44:20 INFO [stats.py:118] GridWorld. Step: 50000. Time Elapsed: 131.649 s. Mean Reward: -0.504. Std of Reward: 0.878. Training.

"process_pixels": {
    "total": 21.636028249000926,
    "count": 45711,
    "is_parallel": true,
    "self": 4.415242508999466,
    "children": {
        "image_decompress": {
            "total": 17.22078574000146,
            "count": 45711,
            "is_parallel": true,
            "self": 17.22078574000146
        }
    }
}

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

Types of change(s)

  • Bug fix
  • New feature
  • Code refactor
  • Breaking change
  • Documentation update
  • Other (please describe)

Checklist

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

Other comments

@chriselion chriselion requested a review from harperj August 27, 2020 03:57
def read(self, size: int = -1) -> bytes:
return self.fp.read(size)

def _real_tell(self) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems odd for this to be _real_tell when it's not used privately / internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, wasn't sure what to call it. Do you think I should remove it and just use fp.tell directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I was just commenting on the initial underscore indicating it is a "private" method. I think "real_tell" is fine, or maybe "force_tell".

return self.fp.read(size)

def _real_tell(self) -> int:
def original_tell(self) -> int:
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 even better

@chriselion chriselion merged commit 2a95006 into master Aug 28, 2020
@delete-merged-branch delete-merged-branch bot deleted the MLA-1306-reduce-copy branch August 28, 2020 18:38
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 28, 2021
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.

2 participants