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

perf: reduce number of avatar requests #10486

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

hamza221
Copy link
Contributor

No description provided.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick review

lib/IMAP/PreviewEnhancer.php Outdated Show resolved Hide resolved
lib/IMAP/PreviewEnhancer.php Outdated Show resolved Hide resolved
lib/IMAP/PreviewEnhancer.php Outdated Show resolved Hide resolved
lib/IMAP/PreviewEnhancer.php Show resolved Hide resolved
@hamza221
Copy link
Contributor Author

hamza221 commented Dec 12, 2024

I broke RecipientBubble 😨, converting to draft

@hamza221 hamza221 marked this pull request as draft December 12, 2024 12:48
@hamza221 hamza221 marked this pull request as ready for review December 12, 2024 14:31
@hamza221
Copy link
Contributor Author

The avatar controller and frontend service are still needed for Recipient bubble (Thread.vue) , because the message object contains only from's avatar ( to, cc, bcc ) still need to be fetched , and assuming that we need them and pre-fetching is counterproductive because we can't assume that all threads are gonna be opened

lib/IMAP/PreviewEnhancer.php Outdated Show resolved Hide resolved
lib/IMAP/PreviewEnhancer.php Outdated Show resolved Hide resolved
lib/IMAP/PreviewEnhancer.php Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
Member

How will this handle worst case scenarios? Imagine you load 20 messages and each of them is sent by someone else, and there is no contacts picture so gravatar favicon sources are tapped. These make external HTTP requests that could take several seconds, e.g. when the favicon request times out because the domain does not correspond to a HTTP server.

Will this slow down the loading of messages?

If so, we have to fine-tune the approach. Let me know, I have some ideas.

@hamza221
Copy link
Contributor Author

So I did a little benchmarking and here are the results

Main

  1. /api/messages?mailboxId=1&limit=20 => 501.22 ms
  2. First batch of requests /api/avatars/url/{email}=> 9583 ms
    total = 10084 ms
  3. external avatars (3 email addresses ) api/avatars/image/{email} => 5158 ms

This branch

  • /api/messages?mailboxId=1&limit=20 (combines 1 and 2 from above) => 1.4 minutes / 2.4 minutes / 2.3 minutes
  • fetching additional messages => 16.94 seconds

Doesn't look like a good approach
Ps: tests done with caching disabling and no additional throttling / timeouts

@ChristophWurst
Copy link
Member

Okay, that is what I was afraid of.

Then we do it like follows: check the cache for an avatar URL. If it exists, attach it to the recipient. Also attach a false if there is one, it indicates that there is no avatar for the user.
Leave the rest of the fetching in tact.

This means that the initial loading with empty cache is still done in many requests, but those help us offload the slow fetching into async operations that don't block the main app functionality. For page reloads, mailbox switches, messages of senders seen previously etc we will preload the avatar URLs and save some requests.

@hamza221
Copy link
Contributor Author

hamza221 commented Dec 23, 2024

I'm not sure how to return a 3rd state "elegantly" ( Avatar => avatar exists , null => avatar doesn't exist, ? => we want to fetch it from the frontend)

https://github.com/nextcloud/mail/pull/10486/files#diff-a7e5eb4728a4ad105af312d21ceb1bd6dd7a70068ba67d4b1881099dcae1076bR99

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

Successfully merging this pull request may close these issues.

2 participants