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

Mainmenu performance regression #14655

Open
grorp opened this issue May 14, 2024 · 6 comments · May be fixed by #14705
Open

Mainmenu performance regression #14655

grorp opened this issue May 14, 2024 · 6 comments · May be fixed by #14705
Assignees
Labels
Bug Issues that were confirmed to be a bug @ Mainmenu Performance Regression Something that used to work no longer does.
Milestone

Comments

@grorp
Copy link
Member

grorp commented May 14, 2024

Minetest version

6303334

The first affected commit is b4be483.

Irrlicht device

SDL

Operating system and version

Fedora Linux 39 (Workstation Edition)

CPU model

Intel® Core™ i7-4790K × 8

GPU model

NVIDIA GeForce GTX 1080 Ti

Active renderer

OpenGL 4.6.0

Summary

While comparing 5.8.0 and 5.9.0-dev, I noticed that the 5.9.0-dev mainmenu reacts noticeably slower to button clicks etc. than the 5.8.0 one in some situations. This is especially visible when looking at the clouds in the background, which make a small jump each time I click a button.

I have bisected this problem to b4be483 / #12208 @rubenwardy

The problem is especially easy to see when going through the pages of the ContentDB dialog. In the following video, 57de599 (good) is on the left and b4be483 (bad) is on the right. For comparability, I used an autoclicker to click with 2 CPS in both videos.

bad.jumping.mp4

Steps to reproduce

In the mainmenu, compare the delay between clicking a button and the result being shown between 5.8.0 and 5.9.0-dev. See that it is longer on 5.9.0-dev and that Minetest may even hang in some cases.

I'm not sure which views are affected by this, but at least the "Content" tab and the ContentDB dialog are definitely affected.

@grorp grorp added Unconfirmed bug Bug report that has not been confirmed to exist/be reproducible Performance @ Mainmenu Regression Something that used to work no longer does. labels May 14, 2024
@rubenwardy
Copy link
Member

rubenwardy commented May 14, 2024

The content translations are cached in memory so this shouldn't be happening. Maybe see if something is erroneously causing the cache to be lost

@Desour
Copy link
Member

Desour commented May 14, 2024

I also noticed this recently (but didn't know it's a regression).
For me, it's especially noticable when you go to the content tab and select something in the "Installed Packets" list.
It probably depends on how many mods and stuff you have installed.
Some profiling and then debugging (Desour@09e8d9c) showed that pkgmgr.update_translations is called multiple times per mod in that menu. And I haven't seen any caching happening.

@Desour Desour added Bug Issues that were confirmed to be a bug and removed Unconfirmed bug Bug report that has not been confirmed to exist/be reproducible labels May 14, 2024
@rubenwardy
Copy link
Member

rubenwardy commented May 14, 2024

The caching is this specifically: https://github.com/minetest/minetest/blob/master/builtin/mainmenu/content/pkgmgr.lua#L792-L803

I'm guessing that something is causing the content to be reloaded unnecessarily

@grorp grorp added this to the 5.9.0 milestone May 14, 2024
@appgurueu
Copy link
Contributor

I don't think there really is any caching? The linked code always translates the title / description. For that it calls an engine function which ends up calling findLocaleFileInMods which hits the file system.

The way that code is currently designed methods like pkgmgr.update_gameslist just update it for all games, and apparently the same for texture packs and mods. Desour seems to be right that, among other things, how "bad" this is depends on how much content you have installed, and I would like to add, probably also on the operating system and type of disk; I would expect that this might be significantly worse e.g. on a HDD, because there all the scattered file system reads really hurt performance.

@rubenwardy
Copy link
Member

The intended caching is that it stores the translated text in the content tables rather than fetching it every time it needs to translate. Something is clearing the content tables unnecessarily

@rubenwardy
Copy link
Member

rubenwardy commented May 25, 2024

Ok, I think it's the update detector that's refreshing the content tables: https://github.com/minetest/minetest/blob/master/builtin/mainmenu/content/update_detector.lua#L130-L131

@rubenwardy rubenwardy self-assigned this May 25, 2024
@rubenwardy rubenwardy linked a pull request May 25, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Issues that were confirmed to be a bug @ Mainmenu Performance Regression Something that used to work no longer does.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants