XWIKI-22540: Slow loading time when changing the icons theme to Silk in the Icon picker #3537

Sereza7 · 2024-10-04T08:38:11Z

Jira URL

https://jira.xwiki.org/browse/XWIKI-22540

Changes

Description

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
Updated slightly the IconPicker javascript to improve readability

Clarifications

This PR is not enough to close the ticket, this represents a 50% reduction in the waiting time but it's still too high. Closing it will happen only when we can figure out a way to speed up loading icons, with https://jira.xwiki.org/browse/XWIKI-11112 for example.

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

Prefers squash: Yes
Backport on branches:
- None, same as XWIKI-12013: Sort the icons alphabetically in the icon picker #3286

…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

mflorea

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).

mflorea · 2024-12-09T09:00:12Z

...-platform-ckeditor/xwiki-platform-ckeditor-plugins/src/main/webjar/xwiki-icon/iconService.js

@@ -52,6 +52,7 @@ define('xwiki-iconService', [
      iconsPromise = iconsPromisesPerTheme[query] = $.getJSON(getResourceURL('data_icons', {
        iconTheme,
        query,
+        'metadata': 'true'


Suggested change

'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?

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 :)

OK, let's keep it like this then, and add a note in the RN.

I think we should cache the icon metadata, and make it fast through the cache, not by excluding the metadata by default.

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

…in the Icon picker * Fix codestyle Co-authored-by: Marius Dumitru Florea <marius@xwiki.com>

…in the Icon picker * Fixed codestyle I didn't rerun all the tests. Co-authored-by: Marius Dumitru Florea <marius@xwiki.com>

.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml

…in the Icon picker * (WIP) * Check for the state of loaded icons and load new icons when the scroll reaches the bottom (see line 309) * Implement an additional cache?

.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml

…in the Icon picker * Implemented a part by part load. * No cache. * Improved various code quality related to comments from this morning.

…in the Icon picker * Improved code style

…in the Icon picker * Improved request action name formatting.

…in the Icon picker * Improved performance of `limit` icon requests.

…in the Icon picker * Escaped special characters

…in the Icon picker * Improved code style

Sereza7 · 2025-01-13T15:32:28Z

Here is the current state of things after 8a7c415 :

2025-01-13.16-21-50.mp4

We can see here that there is no more of a slowness issue. The icons are loaded 20 by 20 and the user can start using the picker before they are all loaded.

When looking at the issue, I figured that the actual use of pagination was quite reduced since most users are already used to searching icons with the input filter. Instead of making everything more complex by adding full blown pagination, I decided to just keep a step by step loading process. This has almost all the advantages of pagination without nearly as much complexity added.

I didn't change the metadata handling compared to before, and I didn't implement a cache. There's already some kind of cache in javascript so we'd need to concile both caches.

Note that on the demo above, my machine's resources were under a lot of stress while recording (and I didn't reboot in a while...), so it was still quite slow. It's nowhere near as annoying as it became and the first render should have a similar speed than what used to be (request to get iconCount + getting 20 icons vs request to get 140 icons).

Sereza7 · 2025-01-13T15:37:06Z

Also, the icon filtering could be done server side (I already added support for this on d80394a).

Getting back on this:
AFAIK this is not what's slowing things down here. Having a full js cache without filter helps to make sure we only query icons once. Removing client side filtering would mean that we'd continue to rely on the network on every change of the input value. IMO the current solution is okay as things are now, so I'd rather not swap this filter responsibility around.

…in the Icon picker * Fixed the spinner not disappearing when changing the icontheme before the initial one is fully loaded.

Sereza7 · 2025-01-13T15:40:08Z

In my manual tests, I tried to make sure that even on low network, interacting with the component while it's in an unstable state wouldn't break it. E.g. changing iconTheme while the iconTheme is not yet fully loaded.

mflorea · 2025-01-28T08:59:08Z

.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml

+    #if ("$!iconNamePrefix" == '' || $stringtool.startsWithIgnoreCase($xwikiIcon, $iconNamePrefix))
+      ## We compare $foreach.count - 1 to our values since foreach.count is 1 based index (!= standard 0 based index)
+      ## We want to make sure we retrieve only the right values.
+      #if (($foreach.count - 1  &gt;= $offset) &amp;&amp; (!$limit || $foreach.count - 1  &lt; $offset + $limit))


Suggested change

#if (($foreach.count - 1 >= $offset) && (!$limit || $foreach.count - 1 < $offset + $limit))

#if ($foreach.index >= $offset && (!$limit || $foreach.index < $offset + $limit))

$foreach.index is also available, and it's 0-based, see https://github.com/search?q=repo%3Axwiki%2Fxwiki-platform+%22foreach.index%22+language%3AXML&type=code&l=XML .

mflorea · 2025-01-28T09:01:53Z

.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml

+    var pageSize = 20;
+    var maxItems = -1;


Suggested change

var pageSize = 20;

var maxItems = -1;

const pageSize = 20;

let maxItems = -1;

mflorea · 2025-01-28T09:02:55Z

.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml

+    var maxItems = -1;
+    // Recursive function helper. This calls the page to retrieve the icon information, by chunk of pageSize.
+    // After receiving the icon info, it displays them and calls itself again if the colortheme is not fully loaded.
+    var partialLoadIcon = function () {


Suggested change

var partialLoadIcon = function () {

function partialLoadIcon() {

mflorea · 2025-01-28T09:04:28Z

.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml

+    // After receiving the icon info, it displays them and calls itself again if the colortheme is not fully loaded.
+    var partialLoadIcon = function () {
+      let itemCount = icons[iconTheme].length;
+      if( maxItems === -1) {


Suggested change

if( maxItems === -1) {

if (maxItems === -1) {

mflorea · 2025-01-28T09:12:40Z

.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml

+        }
+        // Whatever the value of the current icon theme, we finish loading this one so that we have a correct state 
+        // if the user ever comes back to it.
+        if (itemCount + pageSize &lt; maxItems) {


There's no guarantee that maxItems was updated by the first request when this code is called. You have two HTTP requests sent one after another, but there's no guarantee that the response will come in the order the requests were made. It will happen most of the time, but when it doesn't the icon theme will remain partially loaded. I think you should wait for the icon count to be retrieved before starting to fetch the icons (e.g. make partialLoadIcon async and use await), or you could also always include the total count in the response that returns the icons.

mflorea · 2025-01-28T09:14:02Z

.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml

        #set ($icon = {
          'name': $xwikiIcon,
-          'render': $services.icon.renderHTML($xwikiIcon, $iconTheme)
+          'render': $services.icon.renderHTML($xwikiIcon, $iconTheme),
+          'queryString': $iconNamePrefix


Where is this used client-side?

Sereza7 added 3 commits October 2, 2024 17:36

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

96d740e

…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

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

2392ffa

…in the Icon picker * Refactored the IconPicker javascript to improve readability

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

fd1505e

…in the Icon picker * Removed unwanted changes

Sereza7 assigned mflorea Oct 10, 2024

Merge branch 'master' into XWIKI-22540

b50e558

mflorea reviewed Dec 9, 2024

View reviewed changes

Sereza7 and others added 2 commits December 9, 2024 17:43

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

cdc937f

…in the Icon picker * Fix codestyle Co-authored-by: Marius Dumitru Florea <marius@xwiki.com>

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

0053841

…in the Icon picker * Fixed codestyle I didn't rerun all the tests. Co-authored-by: Marius Dumitru Florea <marius@xwiki.com>

mflorea reviewed Dec 10, 2024

View reviewed changes

.../xwiki-platform-icon/xwiki-platform-icon-ui/src/main/resources/IconThemesCode/IconPicker.xml Outdated Show resolved Hide resolved

Sereza7 added 2 commits January 10, 2025 10:38

Merge branch 'xwiki:master' into XWIKI-22540

53b88e0

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

3642613

…in the Icon picker * (WIP) * Check for the state of loaded icons and load new icons when the scroll reaches the bottom (see line 309) * Implement an additional cache?

Sereza7 marked this pull request as draft January 10, 2025 16:56

mflorea reviewed Jan 13, 2025

View reviewed changes

Sereza7 added 7 commits January 13, 2025 15:25

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

1546dc7

…in the Icon picker * Implemented a part by part load. * No cache. * Improved various code quality related to comments from this morning.

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

ec97807

…in the Icon picker * Improved code style

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

4622902

…in the Icon picker * Improved request action name formatting.

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

127d585

…in the Icon picker * Improved request action name formatting.

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

6663d3c

…in the Icon picker * Improved performance of `limit` icon requests.

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

a0087f8

…in the Icon picker * Escaped special characters

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

8a7c415

…in the Icon picker * Improved code style

Sereza7 marked this pull request as ready for review January 13, 2025 15:33

XWIKI-22540: Slow loading time when changing the icons theme to Silk …

4b1f7e1

…in the Icon picker * Fixed the spinner not disappearing when changing the icontheme before the initial one is fully loaded.

Sereza7 requested a review from mflorea January 13, 2025 15:40

mflorea requested changes Jan 28, 2025

View reviewed changes

Sereza7 marked this pull request as draft February 25, 2025 16:05

	#if (($foreach.count - 1 >= $offset) && (!$limit \|\| $foreach.count - 1 < $offset + $limit))
	#if ($foreach.index >= $offset && (!$limit \|\| $foreach.index < $offset + $limit))

	var partialLoadIcon = function () {
	function partialLoadIcon() {

Uh oh!

XWIKI-22540: Slow loading time when changing the icons theme to Silk in the Icon picker #3537

Are you sure you want to change the base?

XWIKI-22540: Slow loading time when changing the icons theme to Silk in the Icon picker #3537

Uh oh!

Conversation

Sereza7 commented Oct 4, 2024

Jira URL

Changes

Description

Clarifications

Screenshots & Video

Executed Tests

Expected merging strategy

Uh oh!

mflorea left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sereza7 commented Jan 13, 2025

Uh oh!

Sereza7 commented Jan 13, 2025

Uh oh!

Sereza7 commented Jan 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!