GroupManager keeps instances of entities forever?

[josephknight]

Howdy JunkDog, hope all is well.

I think I found a leak in GroupManager.

This is the remove method in GroupManganger:

        public void remove(Entity e, String group) {
                Bag<Entity> entities = entitiesByGroup.get(group);
                if(entities != null) {
                        entities.remove(e);
                }

                Bag<String> groups = groupsByEntity.get(e);
                if(groups != null) {
                        groups.remove(group);
                }
        }

Imagine that this entity was only in one group and we are now removing it from that group. Shouldn't we then check to see if it was the last and if so continue to eliminate that Entity, Bag pair from groupsByEntity hasmap? What if we were preparing to delete and dispose of an entity in the game... now the garbage collector can't remove the entity object because it's being held here in groupsByEntity as a key and paired with an empty bag. uh oh.

Am I wrong?

If you look thru all of GroupManager focusing on the groupsByEntity HashMap you may notice that entity keys are only ever 'added' to the map never removed. Surely removal is not automatically done somewhere I'm not noticing.

I would like to fork/add/pr a fix for this if my suspicion is correct, just let me know.

== edited for typos (mixed up groupsByEntity with entitesByGroup) == also: this is a problem for removeFromAllGroups too.

[junkdog]

Ah, nice find! I'm at work atm, so haven't tested it myself, but it indeed appears you're right. Pull requests are always much appreciated :)

Hmm.. Maybe use a BitSet to track which entities have been added to groups? Otherwise, performance will degrade as the number of entities grows.

[josephknight]

I have a very simple but untested fix I've already implemented in my copy of GroupManager, but I'll need to fork and test and PR this for artemis' sake.

A BitSet for groups: I gave it some thought and it doesn't quite seem like the tool for the job in my humble opinion. Maping the group name to a bag of entities and vice-versa seems perfect, there was just an line left out of these methods that prevents a leak.

I may have questions about running tests or I may just ask you to test my PR. I'm pretty busy but am happy to contribute to your repo. Talk w/ you soon.

[josephknight]

Giving more thought to BitSets too... not dismissing that just yet.

[junkdog]

Yeah, the advantage of having a bitset per group would be circumstantial, only making sense when a group contains a myriad of entities (iterating 10k entities shouldn't take more than <1ms on mobile devices, though it's a guesstimate).

If you have maven installed, running the tests is as simple as mvn package from the CLI. Otherwise I could run the tests myself upon merging your PR.

Take care

[josephknight]

PR'd!!!

Shall we continue the discussion at https://github.com/junkdog/artemis-odb/pull/42