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

Improve Kanban performances when loading user pictures #14525

Conversation

AdrienClairembault
Copy link
Contributor

@AdrienClairembault AdrienClairembault commented Apr 14, 2023

Kanban load each users pictures to display using synchronous ajax requests.

This is problematic when you have a lot of tickets with 100+ different user pictures to load.
There is a gif example in the internal ref where the page takes 55 seconds to load because of this, as each images must be loaded one by one while the execution of the page is paused.

@cconard96 Can you confirm that this change (setting the request as asynchronous) is safe ? I see a few more async: false parameters in the same file, can they be removed as well ?

An even better improvement would be to get the picture directly from the main action=refresh ajax request:

image

It seems we already have all information needed in this endpoint, this would help reduce the amount of ajax requests by loading all the data at once.

Is this something you could look into if you have some time ?

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets !27631

@cconard96
Copy link
Contributor

Does this work?
The done method is setting a variable declared before the call is made and then used after the call is made to populate the user badge cache. If it is async, I don't think that would work as expected and it would always end up using a generated badge instead of the image.

This code predates us being allowed to use modern JS features like promises, so I think that is why there are so many synchronous calls.

@AdrienClairembault
Copy link
Contributor Author

Does this work? The done method is setting a variable declared before the call is made and then used after the call is made to populate the user badge cache. If it is async, I don't think that would work as expected and it would always end up using a generated badge instead of the image.

This code predates us being allowed to use modern JS features like promises, so I think that is why there are so many synchronous calls.

I though it was working at first but after a second look it doesn't apply the retrieved images.

What do you think about this part ?

An even better improvement would be to get the picture directly from the main action=refresh ajax request.

It seems we already have all information needed in this endpoint, this would help reduce the amount of ajax requests by loading all the data at once.

Is this something you could look into if you have some time ?

@cconard96
Copy link
Contributor

Also, the Kanban should be making a single call during the initial loading to getUserPicture.php with an array of all user IDs that we would need images for in the preloadBadgeCache method. After a refresh, it also uses this bulk method for any user images not already cached. The only time it should make individual requests is if something tries getting the badge for a user that hasn't had one made yet. It seems like that wouldn't happen very often except maybe when adding team members from the Kanban.

@AdrienClairembault
Copy link
Contributor Author

I see, maybe this is only an issue with the preloadBadgeCache function instead.
In my case it seems to always end up with 0 users to load, exiting the function early.

Will try to look deeper into it, thanks for your insights.

@cconard96
Copy link
Contributor

What do you think about this part ?

An even better improvement would be to get the picture directly from the main action=refresh ajax request.
It seems we already have all information needed in this endpoint, this would help reduce the amount of ajax requests by loading all the data at once.
Is this something you could look into if you have some time ?

Looking at the ticket, I would say the many calls to getUserPicture isn't an expected behavior and even if it was expected, I think the getUserPicture script would need to close the session to make async requests work properly.
It may be possible to load the images in the action=refresh request, but it would still block the loading.

I don't have many users with a picture, but I can add some and look into the issue more.

@AdrienClairembault
Copy link
Contributor Author

AdrienClairembault commented Apr 14, 2023

Seems like there is a code order issue.

The init() function call loadState() THEN build() as a callback.
loadState() will request some users pictures through getTeamBadge(), which wont be able to use the cache as preloadBadgeCache() (which populate the cache) will only be called when the build() function is executed.

image

@AdrienClairembault
Copy link
Contributor Author

AdrienClairembault commented Apr 17, 2023

Latest update: it seems that the issue is that preloadBadgeCache is called before setting self.columns (the cache can't be loaded properly if it doesn't have access to the columns data).

Should be good now, ready for reviews.

@cedric-anne cedric-anne merged commit 690b6e2 into glpi-project:10.0/bugfixes Apr 20, 2023
@AdrienClairembault AdrienClairembault deleted the fix/kanban-userpictures branch October 4, 2023 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants