hmans/miniplex

Garbage collection for Archetypes

Opened this issue · 3 comments

Hello! I stumbled upon this library looking for a good array-of-structs ECS system and I've been working with the for the past month or so in my game. I'm building a renderer using React, but I am not using miniplex-react (frankly, it doesn't seem to provide anything I would use that isn't very easy to implement myself).

I want to be able to render a list of entities matching a criteria, and I would like to be able to use an Archetype for this, however this causes a memory leak. I have a Location component which has a section field, and my React component should render all entities which have a given section. That's easy:

export function useEntities<T>(bucket: ArchetypeBucket<T>): T[] {
  const [_, forceUpdate] = React.useState({});

  React.useEffect(() => {
    const rerender = () => forceUpdate({});
    bucket.onEntityAdded.add(rerender);
    bucket.onEntityRemoved.add(rerender);

    return () => {
      bucket.onEntityAdded.remove(rerender);
      bucket.onEntityRemoved.remove(rerender);
    };
  }, [bucket]);

  return [...bucket];
}

function MyList({ section }) {
  const entities = useEntities(React.useMemo(
    () => world.with("section").where(e => e.location.section === section),
    [section]
  ));
  return (<div>...</div>);
}

This has a memory leak, since it creates a new Archetype every time the section prop changes, but Archetypes are never removed. The feature request is simple: once the last listener for a Bucket is removed, the Bucket should remove its own listeners. If future listeners for the bucket are added, the Bucket should reattach its own listeners and recalculate its entities.

I know I can create a global cache of every section and look up in that cache when rendering my component... but a cache without expiration is still just a memory leak.

Obviously this isn't critical, but since you're changing the API for 2.0 I figured now was an appropriate time to raise this. Thanks for continuing work on this library!

hmans commented

Hi, and thanks for submitting this!

First of all, which version of Miniplex are you using exactly? Your code looks like you're using one of the 2.0 betas — which one?

To be entirely fair, I'm not sure if there really is a memory leak, or if there is one, that it's because of the archetype creation. A call to world.with("section") will be idempotent, only creating the actual archetype on its first call; on subsequent calls, the memoized archetype will be returned. archetype.where(predicate) won't even create any new archetype instances, but just a temporary iterator that allows you to iterate through archetype, with the predicate function applied.

If for some reason multiple instances of the same archetype are being created, that would be a bug. May I ask what steps you took to identify this memory leak? Thanks!

hmans commented

Addendum: I'm honestly surprised that the code example you've posted here works at all, considering you're creating an iterator using where and passing that to your version of useEntities(), which will try to call .onEntityAdded et al on it, which should raise an error because those events don't exist on the iterator object where returns.

I'm assuming you're using 2.0b3 — please let me know if this is wrong.

The implementation of where is here, for reference.

Heads-up: because the nature of how where works has proven a bit iffy to explain to people, it'll be removed in the next beta. If things go as planned, the next beta will also come with a significant reduction in API surface and implementation complexity (but it'll be a breaking change, so be ready.) See #275 for details.

Sorry, I should have mentioned. I am using ^2.0.0-beta.3, which is apparently miniplex@2.0.0-next.20. Also, the code snippet in my original is what I'd like to do, rather than what I am presently doing (which is using a normal archetype and filtering manually). 🫢 Given that you're saying it wouldn't work, well, it wouldn't be a leak either, I guess!

What I really want is to have a component that only rerenders when the entities selected by the predicate change. My actual code presently caches the world.with("section") globally, which is advised in the current README, and then as I mentioned I filter out other entities in my render method. This causes unnecessarily component renders of the list which I was hoping to avoid.

If you're moving towards the API you describe in #275, it would be awesome to have something like this (again, this is just pseudocode):

function MyList({ section }) {
  const entities = useQuery(world, React.useCallback(
    q => q.with("location").where(e => e.location.section === section),
    [section]
  ));
  return (<div>...</div>);
}

If it's possible to create such a useQuery hook, it would greatly reduce my number of rerenders.