matter-ecs / matter

A modern ECS library for Roblox
https://matter-ecs.github.io/matter/
MIT License
70 stars 13 forks source link

Ditch debug.info in the core library, it's slow #133

Open jackTabsCode opened 2 days ago

jackTabsCode commented 2 days ago

Problem

Functions in the library like useHookState (and as a result, all hooks, and queryChanged) and systemName are slow. They use debug.info and each call to debug.info takes between 5-20us depending.

Hooks are highly convenient and very useful, but these performance drawback is rather unfortunate and I think the negatives outweigh the positives, so...

Proposed solution

Ditch debug.info and require users specify a key in useHookState, queryChanged and all hooks.

Discriminators in hooks should still remain optional, as they're often used to differentiate between entities but may not always be necessary.

This is a major breaking change and will require documentation, because using the same key twice in a system will result in unexpected behavior.

christopher-buss commented 2 days ago

There was some work once upon a time to allow users to add a compile step to matter to autogenerate keys so the user didn't need to do this manually, but I don't know if any work was ever done to make this usable in public.

Unless a overall better solution to generating keys, could there be a middleground, that instead of removing the usage of the debug library (for the users that like the ease of it), that you can provide an additional discriminator, that rather than being used as an addition, replaces the call to the debug library if this exists. This would allow those who care for the performance benefits to get them easily, without having to break existing code that relies on the current implementation.

jackTabsCode commented 2 days ago

I personally don't think writing your own keys is that hard, especially when you consider that your keys only need to be unique to the system they're ran in, because a separate topo is created for each system.

christopher-buss commented 2 days ago

I don't think it's necessarily hard either, but for those with existing codebases they would not be able to upgrade to the latest version of matter without adding these keys, for a breaking change that to me could, at least for now, quite easily be opt in.

jackTabsCode commented 2 days ago

We already have a pending breaking change-also, we're still in alpha stages and they should be expected. I feel like this will add code debt we will want to get rid of later anyway, so we might as well do it now while we're in alpha.

LastTalon commented 2 days ago

I never planned on removing the usage entirely. Only to add a way to use it without. I don't think it's a good idea to remove entirely because there are many people who won't use a compile step.

jackTabsCode commented 2 days ago

I don't think a compile step is necessary, because this isn't a particularly difficult change for users to make.

LastTalon commented 20 hours ago

Expecting users to manually supply IDs or use a compile step aren't functionally different for what I said.

I agree with supporting the addition of a way to use it without debug info, but I don't see a compelling reason to remove the existing method. I'd be happy to support this issue if it were amended to be focused on the addition of the new functionality rather than the removal of functionality.