perf(messages): cut redundant work when opening a message - #13359
Open
alexdimarco wants to merge 2 commits into
Open
perf(messages): cut redundant work when opening a message#13359alexdimarco wants to merge 2 commits into
alexdimarco wants to merge 2 commits into
Conversation
alexdimarco
requested review from
ChristophWurst,
GretaD and
kesselb
as code owners
July 25, 2026 20:13
kesselb
reviewed
Jul 25, 2026
kesselb
left a comment
kesselb
left a comment
Contributor
There was a problem hiding this comment.
Thanks
If you used AI for the pull request, please add the Assisted-By trailer (c.f. https://github.com/nextcloud/mail/blob/main/AGENTS.md#commit-message-format).
| $attachmentName = $attachment->getName(); | ||
| if ($attachmentName === null) { | ||
| return new AttachmentDownloadResponse( | ||
| $response = new AttachmentDownloadResponse( |
Contributor
There was a problem hiding this comment.
Move $response behind the if block and just set the attachmentName to the default.
| // Without an adapter, extraction is a no-op. Skip the expensive | ||
| // IMAP download of the message body and all attachments. | ||
| $this->logger->debug('No KItinerary adapter is available, skipping itinerary extraction'); | ||
| $itinerary = new Itinerary(); |
Contributor
There was a problem hiding this comment.
Move Itinerary before the if-block and remove it in line 70.
| // IMAP download of the message body and all attachments. | ||
| $this->logger->debug('No KItinerary adapter is available, skipping itinerary extraction'); | ||
| $itinerary = new Itinerary(); | ||
| $this->cache->set($this->buildCacheKey($account, $mailbox, $id), json_encode($itinerary), self::CACHE_TTL); |
Contributor
There was a problem hiding this comment.
Move $this->buildCacheKey before the if block. Re-use it here and in line 90.
Contributor
|
Thanks for your pr. |
Three targeted reductions of per-click cost, based on profiling the message-open path: - IMAPMessage::getHtmlBody now memoizes the sanitized HTML. getBody called it twice per request (cache write + full message), running the expensive HTMLPurifier pass twice on the same input - ItineraryService::extract checks adapter availability BEFORE opening an IMAP connection and downloading the message body plus all raw attachments. Without a KItinerary adapter installed (the common case), all of that work fed an extractor that is a no-op; the empty result is now computed upfront and cached like a real one - downloadAttachment responses are browser-cacheable for 24h (private, immutable, matching what core's PreviewController does for file previews): attachment content never changes for a given message and attachment id, yet inline images were re-downloaded on every message open, each one paying a fresh IMAP login Covered by new unit tests: memoization (sanitizer runs once per id), itinerary cache hits and the no-adapter skip performing zero IMAP work, and the Cache-Control headers on both attachment branches. Assisted-by: Claude:claude-fable-5 Signed-off-by: Alex DiMarco <alex@dimarcotech.com>
alexdimarco
force-pushed
the
perf/message-open-redundant-work
branch
from
July 25, 2026 20:58
2210caa to
a9b72dc
Compare
Author
|
updated
…On Sat, Jul 25, 2026 at 4:43 PM Daniel ***@***.***> wrote:
*kesselb* left a comment (nextcloud/mail#13359)
<#13359 (comment)>
Thanks for your pr.
—
Reply to this email directly, view it on GitHub
<#13359?email_source=notifications&email_token=ABBN3RTOKO4OWFWQF2PNER35GULXBA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBYGA2TAMBVGEYKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-5080500510>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABBN3RUZ3WYMYLEXDFEN4RD5GULXBAVCNFSNUABEKJSXA33TNF2G64TZHM3DMNZWGIZDEMB3JFZXG5LFHM2DSNZXGUYDINZRHCQXMAQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Alex DiMarco
416-459-0447
|
hasAdapter() duplicated the memoization from extract(), so the $this->findAvailableAdapter() ?? false assignment appeared twice while tests/psalm-baseline.xml only carries one entry for it — static analysis failed on the second occurrence. Look the adapter up in one private helper that hands out a nullable Adapter. Behaviour is unchanged, the false sentinel no longer leaks into the public methods and the psalm baseline stays accurate. Assisted-by: Claude:claude-opus-5 Signed-off-by: Alex DiMarco <alex@dimarcotech.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Profiling the message-open path (click → rendered body) on a deployment with healthy Redis/APCu caching and a LAN IMAP server showed three pieces of pure redundant work dominating server-side cost per click. Related to the broader efforts in #13162 / #13163 but independent of them — these are strict waste-removals with no behavior change.
What this PR does
IMAPMessage::getHtmlBody()is memoized.MessagesController::getBodycalls it twice per request — once for the 600s HTML cache write and again insidegetFullMessage()— running the full HTMLPurifier sanitization twice on identical input. For newsletter-sized HTML that is hundreds of milliseconds of pure CPU per click. The memo is per-instance and id-keyed;IMAPMessageis constructed per message fetch, and its inputs are immutable after construction.ItineraryService::extract()checks adapter availability first. Without a KItinerary adapter installed (the common case), the service still opened an IMAP connection and downloaded the message body plus every raw attachment, only to feed a no-op extractor. It now probes availability before any IMAP work and caches the empty result exactly like a real one (same key, same 7-day TTL) — pre-patch cache semantics are unchanged.downloadAttachmentresponses are browser-cacheable (private, immutable, max-age=86400, matching what core'sPreviewControllerdoes for file previews). Attachment content is immutable for a given message id + attachment id (moves/uidvalidity changes produce new database ids), yet inlinecid:images were re-downloaded on every message open — each one paying a fresh IMAP connection + LOGIN.Test plan