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

Memory leak fix. #156

Merged
merged 2 commits into from
Jul 19, 2019
Merged

Conversation

eaplatanios
Copy link
Contributor

This is pulled out of #152 as @endrift suggested. I'm not sure if it's ok now even though I get no errors when I check it with valgrind (I was getting errors when I wasn't using strdup). Who is supposed to own the memory pointed to by data and free it? @endrift do you have any suggestions?

@christopherhesse
Copy link
Collaborator

christopherhesse commented Apr 19, 2019

It's not clear to me what RETRO_ENVIRONMENT_GET_SYSTEM_DIRECTORY is used for, could this just return like the path to the directory that the retro shared object is in? If it returns some sort of global constant, retro can easily own the memory for the duration of the process.

Copy link
Contributor

@endrift endrift left a comment

Choose a reason for hiding this comment

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

I'd cache the strdup'd version on the Emulator class (check if it's null and call corePath() if it is) and delete it at destruction time. It never changes after the first call so this is safe. That said, because it's not changed I'm not sure the buffer will ever be invalidated, so the original version may already be safe. It's not entirely clear to me, given C++'s wishy-washy memory safety guarantees.

@eaplatanios
Copy link
Contributor Author

@endrift @christopherhesse I made the requested change. Could you please take another look?

Also, what is role of atari-py relative to retro? If one wants to play around with atari environments which one do you recommend using and why?

@christopherhesse
Copy link
Collaborator

Retro is nice if you want consistency with other retro games, but ALE is faster, see #127

@eaplatanios
Copy link
Contributor Author

Thanks for the quick response! Are there plans to tackle the issues mentioned in that post? Also, when you say faster, what kind of difference have you measured? Is it significant?

@christopherhesse
Copy link
Collaborator

christopherhesse commented Jul 19, 2019

I don't remember, but I believe it is significant, something like 2-3x faster per frame for ALE. If you use a large model or have a lot of CPUs, maybe it doesn't matter as much, but there's definitely a speed difference.

Copy link
Contributor

@endrift endrift left a comment

Choose a reason for hiding this comment

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

lgtm now

@christopherhesse
Copy link
Collaborator

Thanks @endrift and @eaplatanios!

@christopherhesse christopherhesse merged commit fced6f2 into openai:master Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants