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

Juggler: Reduced memory allocations by removing the Dictionary implementation for object ids #1099

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

Adolio
Copy link
Contributor

@Adolio Adolio commented Jul 4, 2022

This should prevent many String allocations

Changes

  • The _objectIDs is now a Vector.<uint> containing the objects' ids
  • The cells cleaning logic as been factorized by using the new removeByIndex method
  • object in _objectIDs have been replaced by contains(object)
  • The cells re-oganization (in the advanceTime method) logic has been updated accordingly

This PR needs further testing. Changes are working on my side but I'm not sure I have covered all cases.

…entation for object ids

This should prevent many String allocations
@PrimaryFeather
Copy link
Contributor

PrimaryFeather commented Jul 8, 2022

This is really an exemplary pull request, Aurélien! Thanks a lot!

I really like how you got rid of the ugly code duplication I had in there whenever an object was removed. I'm actually a little embarrassed that it wasn't implemented like that originally! 🫢

Thankfully, the Juggler class is extensively unit tested – there are a few traps in there which can easily be overlooked, but I think all of them are covered by the unit tests. And those are running just fine after merging your changes.

So, everything is looking great! Thanks a lot for your careful implementation, Aurélien! 😄👏

@Adolio
Copy link
Contributor Author

Adolio commented Jul 10, 2022

Thanks Daniel for your careful review 😁

I'm very happy that you liked it, this means a lot to me coming from you! ☺️

@Adolio Adolio deleted the juggler-memory-imp branch July 19, 2022 16:29
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.

None yet

2 participants