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

Fix unnecessary content refreshing #14705

Conversation

rubenwardy
Copy link
Member

@rubenwardy rubenwardy commented May 25, 2024

Fixes #14655

To do

This PR is Ready for Review

How to test

  • Use the content tab and the mainmenu. Install, remove, update content. Use settings
function pkgmgr.update_translations(list)
	core.log("error", "Updating translations for " .. #list .. " items")
	core.log(debug.traceback())

@rubenwardy rubenwardy requested a review from grorp May 25, 2024 21:02
@rubenwardy rubenwardy added this to the 5.9.0 milestone May 25, 2024
@rubenwardy rubenwardy marked this pull request as ready for review May 25, 2024 21:28
@rubenwardy
Copy link
Member Author

rubenwardy commented May 25, 2024

This is ready to review as-is, but it may not fix the issue 100%. If this is a problem, there are two ideas I have to do here:

  • The texture packs list is currently not cached, meaning that the translations will be loaded every time. So we could cache this. Edit: done

  • We could do translation caching, caching the values set by update_translations by the content path. We'd need to make sure to invalidate translations when updating or removing, including any child resources

    +local translation_cache = {}
    +function pkgmgr.invalidate_translation(path)
    +       local abs_path = core.get_absolute_path(path)
    +       local last_char = abs_path:sub(#abs_path)
    +       -- Remove trailing / as content paths don't have them.
    +       -- This can result in unintended invalidation, but it's not a problem
    +       if last_char == "/" or last_char == "\\" then
    +               abs_path = abs_path:sub(#abs_path - 1)
    +       end
    +       for key, value in pairs(translation_cache) do
    +               if key:sub(1, #abs_path) == abs_path then
    +                       translation_cache[key] = nil
    +               end
    +       end
    +end
     function pkgmgr.update_translations(list)
            for _, item in ipairs(list) do
                    local info = core.get_content_info(item.path)
                    assert(info.path)
                    assert(info.textdomain)
                    assert(not item.is_translated)
                    item.is_translated = true
     
    +               local key = core.get_absolute_path(info.path)
    +               local cache = translation_cache[key]
    +               if not cache then
    +                       core.log("error", "Cache miss for " .. info.path)
    +                       cache = {}
    +                       cache.title = info.title and info.title ~= "" and
    +                               core.get_content_translation(info.path, info.textdomain,
    +                                       core.translate(info.textdomain, info.title))
    +                       cache.description = info.description and info.description ~= "" and
    +                               core.get_content_translation(info.path, info.textdomain,
    +                                       core.translate(info.textdomain, info.description))
    +                       translation_cache[info.path] = cache
    +               end
    +
    -               if info.title and info.title ~= "" then
    -                       item.title = core.get_content_translation(info.path, info.textdomain,
    -                               core.translate(info.textdomain, info.title))
    -               end
    -               if info.description and info.description ~= "" then
    -                       item.description = core.get_content_translation(info.path, info.textdomain,
    -                               core.translate(info.textdomain, info.description))
    -               end
    +               item.title = cache.title or item.title
    +               item.description = cache.description or item.description
            end
     end

Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already much better.

I suggest this, it makes the debug output more readable by updating translations for all mods at once:

diff --git a/builtin/mainmenu/content/pkgmgr.lua b/builtin/mainmenu/content/pkgmgr.lua
index 720c435c4..891421100 100644
--- a/builtin/mainmenu/content/pkgmgr.lua
+++ b/builtin/mainmenu/content/pkgmgr.lua
@@ -150,8 +150,6 @@ function pkgmgr.get_mods(path, virtual_path, listing, modpack)
                        toadd.virtual_path = mod_virtual_path
                        toadd.type = "mod"
 
-                       pkgmgr.update_translations({ toadd })
-
                        -- Check modpack.txt
                        -- Note: modpack.conf is already checked above
                        local modpackfile = io.open(mod_path .. DIR_DELIM .. "modpack.txt")
@@ -176,6 +174,8 @@ function pkgmgr.get_mods(path, virtual_path, listing, modpack)
                table.sort(listing, function(a, b)
                        return a.virtual_path:lower() < b.virtual_path:lower()
                end)
+
+               pkgmgr.update_translations(listing)
        end
 end
 

I think it would be good to cache texture packs as well. Basically pkgmgr.get_all() is called on every single formspec interaction because the "Content" tabheader includes an update count.

ERROR[Main]: Updating translations for 83 items
[Main]: stack traceback:
	/mt/builtin/mainmenu/content/pkgmgr.lua:795: in function 'update_translations'
	/mt/builtin/mainmenu/content/pkgmgr.lua:194: in function 'get_texture_packs'
	/mt/builtin/mainmenu/content/pkgmgr.lua:220: in function 'get_all'
	/mt/builtin/mainmenu/content/update_detector.lua:128: in function 'get_all'
	/mt/builtin/mainmenu/content/update_detector.lua:151: in function 'get_count'
	/mt/builtin/mainmenu/tab_content.lua:286: in function 'caption'
	/mt/builtin/fstk/tabview.lua:164: in function 'tab_header'
	/mt/builtin/fstk/tabview.lua:86: in function 'get_formspec'
	/mt/builtin/fstk/ui.lua:101: in function 'update'
	/mt/builtin/fstk/ui.lua:145: in function 'handle_buttons'
	/mt/builtin/fstk/ui.lua:186: in function /mt/builtin/fstk/ui.lua:172>

builtin/mainmenu/content/contentdb.lua Outdated Show resolved Hide resolved
@grorp
Copy link
Member

grorp commented May 26, 2024

Another problem is that the mod list is still loaded twice on startup. A log excerpt: log.txt

@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jun 7, 2024
@rubenwardy
Copy link
Member Author

Another problem is that the mod list is still loaded twice on startup. A log excerpt: log.txt

Looks like this is due to the settingtypes loading code

@rubenwardy rubenwardy force-pushed the fix-mainmenu-content-translation-slowness branch from 811c415 to 0a5ac08 Compare June 9, 2024 09:57
@rubenwardy rubenwardy marked this pull request as draft June 9, 2024 09:58
@rubenwardy rubenwardy force-pushed the fix-mainmenu-content-translation-slowness branch from 0a5ac08 to 547f1a4 Compare June 9, 2024 10:06
@rubenwardy rubenwardy added Testing needed and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Jun 9, 2024
@rubenwardy rubenwardy marked this pull request as ready for review June 9, 2024 10:21
@rubenwardy rubenwardy force-pushed the fix-mainmenu-content-translation-slowness branch from 5e08bed to 429f868 Compare June 9, 2024 10:34
@rubenwardy rubenwardy force-pushed the fix-mainmenu-content-translation-slowness branch from 429f868 to f627bf8 Compare June 9, 2024 10:34
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enabling/disabling texture packs doesn't work properly

@grorp grorp added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jun 9, 2024
@rubenwardy rubenwardy removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Jun 9, 2024
@rubenwardy rubenwardy requested a review from grorp June 9, 2024 19:49
Copy link
Member

@grorp grorp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested:

  • uninstall reloads by type correctly
  • update reloads by type correctly
  • install reloads by type correctly
  • enabling/disabling texture packs works (and reloads, I guess that would be avoidable)
  • builtin/game/mod/client mod settings are loaded

builtin/mainmenu/content/pkgmgr.lua Outdated Show resolved Hide resolved
builtin/mainmenu/settings/dlg_settings.lua Show resolved Hide resolved
@rubenwardy rubenwardy merged commit 157d129 into minetest:master Jun 24, 2024
2 checks passed
@rubenwardy rubenwardy deleted the fix-mainmenu-content-translation-slowness branch June 24, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mainmenu performance regression
3 participants