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

[BUG] Unnecessary allocation in ZInput.GetButtion() & ZInput.GetButtonDown() #385

Open
java-devil opened this issue Jan 16, 2023 · 1 comment

Comments

@java-devil
Copy link

java-devil commented Jan 16, 2023

Details:
Jotunn Version: 2.10.3
Jotunn Submodule(if applicable): InputManager
Repeatability(Consistent(100%), Inconsistent(50%), Rare(1%), Unknown(==1)): 100%

Problem Description:
Jotunn.dll!Jotunn.Managers::InputManager.ZInput_GetButton() & Jotunn.dll!Jotunn.Managers::InputManager.ZInput_GetButtonDown() allocate trash (due to concatenation) potentially dozens of times per frame (especially if the critical flow of the mod depends on key presses... as it ofc does for OCDheim 😉). They allocate only ~60B per invocation, so this is more of "makes profiling more difficult to analyze" than "performacne issue".

Expected Behaviour:
I would hope there to be no allocation 😉. If given guidance on prefered implementation - I'm perfectly willing to provide a PR.

@java-devil java-devil changed the title [BUG] [BUG] Unnecessary allocation in ZInput.GetButtion() & ZInput.GetButtonDown() Jan 16, 2023
@MSchmoecker
Copy link
Member

Yes, I also don't think the impact on performance is that high, otherwise we likely would have noticed it before. But without dedicated profiling, I cannot say this with full certainty. So yeah, it makes sense to change the calls to allocate less memory if possible and has some impact.

If you want to provide a PR this is perfectly fine. As it doesn't seem to have a high priority, I (and probalby Jules, too) won't get to it in the short term.

The last major changes to the input system were with the mmhook remove. Porting the hooks to Harmony required some things like reverse patching, but I didn't evaluate if that was the best solution at the time. It was one of the more difficult ports and getting it to work at all was more important.

So I think a rework of the interal systems are appropriate anyway, but if you only want to fix the string concatination allocation this is alright of course. In general, everything that is done without changing the public API is fine, this means both public interfaces and functioning wise. In the case of the input system, if it works slightly different but still makes and doesn't break mods it's also okay I would say. But this must be evaluated if it occurs.

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

No branches or pull requests

2 participants