XWIKI-22540: Slow loading time when changing the icons theme to Silk in the Icon picker #3537
Conversation
…in the Icon picker * Added a flag on the data_icons action to only retrieve metadata when necessary. This shaves off half of the time for the request and reduces significantly the size of the payload
…in the Icon picker * Refactored the IconPicker javascript to improve readability
…in the Icon picker * Removed unwanted changes
There was a problem hiding this comment.
Adding a flag for the metadata is a good idea, but I think the real fix is to add pagination to the icon picker. Loading more icons could be triggered by scroll or by clicking on a "Load more..." link. Also, the icon filtering could be done server side (I already added support for this on d80394a).
| @@ -52,6 +52,7 @@ define('xwiki-iconService', [ | |||
| iconsPromise = iconsPromisesPerTheme[query] = $.getJSON(getResourceURL('data_icons', { | |||
| iconTheme, | |||
| query, | |||
| 'metadata': 'true' | |||
There was a problem hiding this comment.
| 'metadata': 'true' | |
| metadata: true |
I'm wondering if this can't be considered a breaking change by those (extensions) that use the icon picker data_icons URL as an API. Sure, it's not as public / advertised as the icon REST API, but wouldn't it be safer to return the icon metadata by default (i.e. keep the current behavior) and to pass metadata: false when you don't need it?
There was a problem hiding this comment.
Those metadatas were added in 15.7, with https://jira.xwiki.org/browse/XWIKI-20979.
It's likely that most uses of this API in the extensions/user customizations do not need or want those metadatas and got significantly slower when we added them by default. IMO the default should still be without it: in our use cases it has been reported as a performance regression, and I think it would end up similarly for most customizations.
At the end of the day, we need to choose one:
- Break extensions made between 15.7 and 16.10 that rely on the metadata.
- Have all extensions relying on this rest API being twice as slow as they were before 15.7
Note that if we chose the first one, we can add a note about it in the release notes, and with the appropriate documentation update, it should be quite straightforward to fix for the impacted extension maintainers.
I'm keeping the PR as is for now, let me know if you think my argumentation and choice is not correct and I'll update it :)
There was a problem hiding this comment.
OK, let's keep it like this then, and add a note in the RN.
There was a problem hiding this comment.
I think we should cache the icon metadata, and make it fast through the cache, not by excluding the metadata by default.
There was a problem hiding this comment.
For me the priority should be to add some kind of pagination to the icon picker because it doesn't scale currently. Adding the icon metadata cache is nice, but I'm not sure if it will solve the slowness (for an image-based icon theme like Silk where the icon picker needs to fetch and display 1k+ image icons).
.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml
Outdated
Show resolved
Hide resolved
.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml
Outdated
Show resolved
Hide resolved
.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml
Outdated
Show resolved
Hide resolved
…in the Icon picker * Fix codestyle Co-authored-by: Marius Dumitru Florea <[email protected]>
…in the Icon picker * Fixed codestyle I didn't rerun all the tests. Co-authored-by: Marius Dumitru Florea <[email protected]>
| @@ -243,8 +243,7 @@ require(['jquery', 'xwiki-icon-picker'], function($) { | |||
| selectedIconName = currentInput[0].value.replace(currentInput.data('xwikiIconPickerSettings').prefix, ''); | |||
| } | |||
| // For each icon | |||
| for (var i=0; i < iconList.length; ++i) { | |||
| let icon = iconList[i]; | |||
| iconList.forEach(icon => { | |||
There was a problem hiding this comment.
You need a closing ) at the end (I couldn't add it as a code suggestion :) ).
Jira URL
https://jira.xwiki.org/browse/XWIKI-22540
Changes
Description
Clarifications
Screenshots & Video
N/A, performance issue only
Executed Tests
Manual tests, time to display was reduced by half. Locally, the request before the changes would take about 5 seconds. This was most of the waiting time. After the PR, this request was only half as long (2 to 2.5s).
Expected merging strategy