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

Avoid unnecessary API requests #1845

Closed
wants to merge 27 commits into from
Closed

Avoid unnecessary API requests #1845

wants to merge 27 commits into from

Conversation

Dymirt
Copy link

@Dymirt Dymirt commented Jun 5, 2023

In the fetchFaces method, we can remove the check if (Object.keys(this.faces).length) before making the API request. Since the fetchFaces method is called in the beforeMount hook, it will already be executed only once during the component's lifecycle. Therefore, we don't need to check if the faces data is already present in the Vuex store before making the API request.

@Dymirt
Copy link
Author

Dymirt commented Jun 5, 2023

By combining the operations into a single loop, we reduce the number of iterations over the array and minimize unnecessary processing, leading to improved performance.

@artonge artonge requested a review from marcelklehr June 5, 2023 14:53
@artonge
Copy link
Collaborator

artonge commented Jun 5, 2023

Is the improvement really noticeable?

@artonge
Copy link
Collaborator

artonge commented Jun 5, 2023

Also, it does not look like we are doing less API requests.

@Dymirt
Copy link
Author

Dymirt commented Jun 5, 2023

Also, it does not look like we are doing less API requests.

You're right. there is same amount of an API requests. But using loop is more readable in first, additionally, consolidating the operations into a single loop can improve memory usage by avoiding the creation of intermediate arrays. This can be beneficial in scenarios where memory consumption is a concern.

This is my first commit in to nextcloud, so im trying to make some small changes. On my nextcloud I have more than 100 people recognised and some of them have more than 1000 photos. So im trying to make les requests, and more efficient loading sice there is too many requests and all photos per person is loading on main Persons page.
My logs looks like:
[Debug] [DEBUG] photos: [FetchFacesMixin] Fetched 1235 new files: – {0: "99030", 1: "99159", 2: "99160", …} (photos-main.js, line 2)

@artonge
Copy link
Collaborator

artonge commented Jun 5, 2023

all photos per person is loading on main Persons page.

Indeed, any reason we are doing this @marcelklehr?

@marcelklehr
Copy link
Member

marcelklehr commented Jun 5, 2023

Thank you for looking into making the faces views more performant @Dymirt 💙

I think the real gold nugget is this line here: https://github.com/nextcloud/photos/blob/master/src/components/Faces/FaceCover.vue#L115

We should try to only load the files per face when that face is actually opened. the face directories should already have a nbItems property that contains the number of faces, but it doesn't have any properties for the cover images. I guess it would be best if recognize supplies that. I'll come up with a way to add that, then we can implement this. Feel free to hop by in https://matrix.to/#/#marcelklehr_recognize:gitter.im to coordinate on this if you like :) 👋

@marcelklehr
Copy link
Member

Indeed, any reason we are doing this @marcelklehr?

lack of foresight :D

@Dymirt
Copy link
Author

Dymirt commented Jun 6, 2023

@marcelklehr Can you please do code review?

@marcelklehr
Copy link
Member

It seems @artonge has been refactoring the photos frontend, so we could now reuse his collections interfaces for faces.

@marcelklehr
Copy link
Member

@Dymirt DCO is still missing. Could you sign your commits?

@artonge
Copy link
Collaborator

artonge commented Jun 6, 2023

It seems @artonge has been refactoring the photos frontend, so we could now reuse his collections interfaces for faces.

More, is coming with #1621 for stores, should be merged in a week or so.

@Dymirt
Copy link
Author

Dymirt commented Jun 6, 2023

@Dymirt DCO is still missing. Could you sign your commits?

I have trouble with sign my previous commits.

Dymirt and others added 15 commits June 7, 2023 11:09
…).length) before making the API request. Since the fetchFaces method is called in the beforeMount hook, it will already be executed only once during the component's lifecycle. Therefore, you don't need to check if the faces data is already present in the Vuex store before making the API request.

Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Marcel Klehr <mklehr@gmx.net>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
artonge and others added 12 commits June 7, 2023 11:09
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Louis Chemineau <louis@chmn.me>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Nextcloud bot <bot@nextcloud.com>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
Signed-off-by: Dymirt <kolidadmytro@gmail.com>
@Dymirt
Copy link
Author

Dymirt commented Jun 7, 2023

Fix/avoid unnecessary api requests #1860

@Dymirt Dymirt closed this Jun 7, 2023
@marcelklehr
Copy link
Member

More, is coming with #1621 for stores, should be merged in a week or so.

Alright, then let's get this PR in before that and wait for @artonge 's PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants