Defect #44412
closed
Themes with custom icons.svg slow down page rendering
Description
When the current theme provides its own icons.svg using the feature introduced in #43087, sprite_source calls Theme#icons for every icon rendered. Each call fetches the list of icon names from ActionController::Base.cache_store, which is costly when repeated for every icon. With the default file store, one fetch costs about 340 µs in my development environment.
I measured the time spent in these lookups with a third-party theme under the following conditions:
| Page | Rendered icons | Without icons.svg | With icons.svg | With icons.svg and patch |
|---|---|---|---|---|
| Typical page (issue list, project settings, etc.) | 20-65 | ~0 ms | 10-20 ms | < 1 ms |
| Issue page with 30 journals and 30 attachments | ~370 | ~0.2 ms | ~120 ms | ~5 ms |
Themes without their own icons.svg are not affected because Theme#icons returns before accessing the cache store.
The attached patch replaces the cache-store lookup with memoization in the Theme object, keyed by the asset digest so that changes to the sprite file are still picked up. Theme objects are shared by all requests in the process, so the memoized names remain available for the lifetime of the process.
The main benefit of keeping the cache store would be that a shared cache can avoid parsing the sprite separately in each process. However, parsing is needed only once per process and takes less than 0.4 ms in my development environment. The memoized icon-name list is also small, so removing the cache store has no significant downside.
Files
Related issues
Updated by Marius BĂLTEANU 18 days ago
- Related to Feature #43087: Allow to change icons sprites from theme added
Updated by Marius BĂLTEANU 17 days ago
Go MAEDA, I've committed the fix, thanks for taking care of these performance issues.
What do you think if we also cache the calls made by each icon for sprite path?
diff --git a/app/helpers/icons_helper.rb b/app/helpers/icons_helper.rb
index 4d21ebedf..7ee3869ab 100644
--- a/app/helpers/icons_helper.rb
+++ b/app/helpers/icons_helper.rb
@@ -61,7 +61,7 @@ module IconsHelper
def sprite_source(icon_name, sprite: DEFAULT_SPRITE, plugin: nil)
if plugin
"plugin_assets/#{plugin}/#{sprite}.svg"
- elsif current_theme && current_theme.icons(sprite).include?(icon_name)
+ elsif current_theme && theme_icon_set(sprite).include?(icon_name)
current_theme.image_path("#{sprite}.svg")
else
"#{sprite}.svg"
@@ -178,4 +178,9 @@ module IconsHelper
MIME_TYPE_ICONS[mime.to_s.split('/').first] ||
'file'
end
+
+ def theme_icon_set(sprite)
+ @theme_icon_sets ||= {}
+ @theme_icon_sets[sprite] ||= Set.new(current_theme&.icons(sprite))
+ end
end
Updated by Go MAEDA 17 days ago
Marius BĂLTEANU wrote in #note-4:
Go MAEDA, I've committed the fix, thanks for taking care of these performance issues.
What do you think if we also cache the calls made by each icon for sprite path?
[...]
Thank you for the suggestion, it is a nice improvement: it avoids repeated calls to current_theme.icons, and looking up an icon name in a Set is also faster than scanning an array of more than a hundred names.
Updated by Marius BĂLTEANU 16 days ago
- Status changed from New to Closed
- Assignee set to Marius BĂLTEANU
- Resolution set to Fixed
Committed and merged to 7.0-stable branch.