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

Fix memory leak between the RLBot library and rlbot-rust #29

Closed
wants to merge 15 commits into from

Conversation

saintdev
Copy link
Collaborator

@saintdev saintdev commented Mar 7, 2020

BREAKING CHANGE

Fixes #25 a memory leak where the byte buffer passed from the RLBot interface library was never freed. This requires quite drastic internal change to how serialized flatbuffers are handled. Because it is now required to deserialize into our own types, this will break existing Rust bots.

Concerns

  • The need to immediately deserialize to our own types will break existing bots.
  • There is an extra memory allocation and copy, so we can immediately free the ByteBuffer passed from the RLBot library.
  • External types src/game.rs
    • Can easily desync from their upstream ffi and flatbuffer types (src/ffi.rs and src/rlbot_generated.rs)
      • Can this be automated?
    • Are manually defined, instead of being derived from an upstream source.

I'm also open to rewriting this with a flatbuffer wrapper type that Deref's to the underlying flatbuffer type. I discarded this solution, because it means the wrapper has to carry around a reference to RLBotCoreInterface::free(). Overall, the current solution seems cleaner, at the expense of an allocation and a copy.

@saintdev saintdev added the bug Something isn't working label Mar 7, 2020
@saintdev saintdev requested a review from Quetzal2 March 7, 2020 03:17
@saintdev saintdev self-assigned this Mar 7, 2020
@kipje13
Copy link

kipje13 commented Mar 7, 2020

I'm also open to rewriting this with a flatbuffer wrapper type that Deref's to the underlying flatbuffer type. I discarded this solution, because it means the wrapper has to carry around a reference to RLBotCoreInterface::free().

Would it be possible to copy the flatbuffer data into a vector that is owned by the wrapper? It should then be possible to immediately free the original buffer so you don't have to carry around a reference to RLBotCoreInterface::free(). This has the same downside that you need allocate and copy some memory. But on the other hand it will only require slight changes to existing bots and does not require manual updates when the flatbuffers schemas are updated.

@Quetzal2
Copy link
Collaborator

Quetzal2 commented Mar 9, 2020

Nice!
I agree, maintaining the types manually is a bit annoying, and Kipje's take on it looks very interesting.
I'm ok to go with this version first to fix the leak. We can always roll out an improved version later on. And I don't think that porting the existing bots once again will be a big task (there's not a lot of bots using it at the moment either), so there's no special constraints concerning a future improved version, if we want one.

@saintdev saintdev closed this in bbbdd3e May 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak
3 participants