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

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

Open
jackTabsCode opened this issue Oct 29, 2024 · 7 comments
Open

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

jackTabsCode opened this issue Oct 29, 2024 · 7 comments
Labels
breaking Introduces a breaking change improvement An imperfection we can make better

Comments

@jackTabsCode
Copy link
Contributor

jackTabsCode commented Oct 29, 2024

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.

@jackTabsCode jackTabsCode added breaking Introduces a breaking change improvement An imperfection we can make better labels Oct 29, 2024
@christopher-buss
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@LastTalon
Copy link
Member

LastTalon commented Oct 31, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Introduces a breaking change improvement An imperfection we can make better
Projects
None yet
Development

No branches or pull requests

3 participants