This reverts commit c56649fe5291b725f14b45550a68cc7d0dc16ff7.
As discussed in the aerc-devel thread for the reverted patch, the
performance improvement comes with an issue: changes to message tags are
no longer immediately reflected in the UI. This issue occurs whether the
tags are modified from within aerc or externally with the notmuch CLI.
The message list also flickers any time tag changes are made.
Further, commit c36ed72e4a59 ("notmuch: speed up lastmod query")
dramatically reduces the number of messages which are re-indexed when
the database changes, likely eliminating the need for the reverted
performance improvement anyway.
Fixes: c56649fe5291 ("notmuch: don't reload all message on change")
Link: https://lists.sr.ht/~rjarry/aerc-devel/patches/53729
Link: https://lists.sr.ht/~rjarry/aerc-devel/patches/54028
Reported-by: Robin Dapp <rdapp@modk.org>
Reported-by: Ryan Winograd <ryan@thewinograds.com>
Cc: Hugo Osvaldo Barrera <hugo@whynothugo.nl>
Signed-off-by: Jason Cox <me@jasoncarloscox.com>
---
worker/notmuch/eventhandlers.go | 15 ++++++++--
worker/notmuch/worker.go | 49 +++++++++++++++++++--------------
2 files changed, 41 insertions(+), 23 deletions(-)
diff --git a/worker/notmuch/eventhandlers.go b/worker/notmuch/eventhandlers.go
index e4c2536e..fb454e08 100644
--- a/worker/notmuch/eventhandlers.go
+++ b/worker/notmuch/eventhandlers.go
@@ -9,6 +9,7 @@ import (
"path/filepath"
"strconv"
+ "git.sr.ht/~rjarry/aerc/lib/log"
"git.sr.ht/~rjarry/aerc/worker/types"
)
@@ -75,9 +76,17 @@ func (w *worker) updateChangedMessages() error {
if err != nil {
return fmt.Errorf("Couldn't get updates messages: %w", err)
}
- w.w.PostMessage(&types.DirectoryContents{
- Uids: uids,
- }, nil)
+ for _, uid := range uids {
+ m, err := w.msgFromUid(uid)
+ if err != nil {
+ log.Errorf("%s", err)
+ continue
+ }
+ err = w.emitMessageInfo(m, nil)
+ if err != nil {
+ log.Errorf("%s", err)
+ }
+ }
w.state = newState
return nil
}
diff --git a/worker/notmuch/worker.go b/worker/notmuch/worker.go
index 249347ce..c6a81050 100644
--- a/worker/notmuch/worker.go
+++ b/worker/notmuch/worker.go
@@ -433,30 +433,12 @@ func (w *worker) handleFetchMessageHeaders(
w.emitMessageInfoError(msg, uid, err)
continue
}
- info, err := m.MessageInfo()
+ err = w.emitMessageInfo(m, msg)
if err != nil {
+ w.w.Errorf("could not emit message info: %v", err)
w.emitMessageInfoError(msg, uid, err)
continue
}
- switch {
- case len(w.headersExclude) > 0:
- info.RFC822Headers = lib.LimitHeaders(info.RFC822Headers, w.headersExclude, true)
- case len(w.headers) > 0:
- info.RFC822Headers = lib.LimitHeaders(info.RFC822Headers, w.headers, false)
- }
-
- switch msg {
- case nil:
- w.w.PostMessage(&types.MessageInfo{
- Info: info,
- }, nil)
- default:
- w.w.PostMessage(&types.MessageInfo{
- Message: types.RespondTo(msg),
- Info: info,
- }, nil)
- }
-
}
w.done(msg)
return nil
@@ -698,6 +680,33 @@ func (w *worker) emitMessageInfoError(msg types.WorkerMessage, uid uint32, err e
}, nil)
}
+func (w *worker) emitMessageInfo(m *Message,
+ parent types.WorkerMessage,
+) error {
+ info, err := m.MessageInfo()
+ if err != nil {
+ return fmt.Errorf("could not get MessageInfo: %w", err)
+ }
+ switch {
+ case len(w.headersExclude) > 0:
+ info.RFC822Headers = lib.LimitHeaders(info.RFC822Headers, w.headersExclude, true)
+ case len(w.headers) > 0:
+ info.RFC822Headers = lib.LimitHeaders(info.RFC822Headers, w.headers, false)
+ }
+ switch parent {
+ case nil:
+ w.w.PostMessage(&types.MessageInfo{
+ Info: info,
+ }, nil)
+ default:
+ w.w.PostMessage(&types.MessageInfo{
+ Message: types.RespondTo(parent),
+ Info: info,
+ }, nil)
+ }
+ return nil
+}
+
func (w *worker) emitLabelList() {
tags := w.db.ListTags()
w.w.PostMessage(&types.LabelList{Labels: tags}, nil)
--
2.46.0
On Wed Aug 7, 2024 at 5:07 PM EDT, Bence Ferdinandy wrote:
> 2024. aug. 7. 22:57:04 Jason Cox <me@jasoncarloscox.com>:
>
> > This reverts commit c56649fe5291b725f14b45550a68cc7d0dc16ff7.
>
> Shouldn't reverting be a revert commit by Robin?
I don't know; I didn't see the revert process documented anywhere, and I
thought the patch should be reverted, so I sent my own revert commit.
Totally fine by me if Robin wants to handle it, though!
On Wed, 7 Aug 2024, at 22:55, Jason Cox wrote:
> This reverts commit c56649fe5291b725f14b45550a68cc7d0dc16ff7.
>
> [...snip...]
Somebody else reported similar issues on IRC. I have no objection to
reverting this until we can properly debug what's going on (I don't have
the time to do so right now right now).
For context, without my patch each time I mark a message as seen or
deleted a message, I'd get CPU spike at 150% for ~60 seconds. With this
patch, the operation took just 2s. Others couldn't properly reproduce my
issue, so I'll open a ticket to track it properly before proposing
another patch.
--
Hugo
On Thu Aug 8, 2024 at 4:49 AM EDT, Hugo Osvaldo Barrera wrote:
> For context, without my patch each time I mark a message as seen or
> deleted a message, I'd get CPU spike at 150% for ~60 seconds. With this
> patch, the operation took just 2s. Others couldn't properly reproduce my
> issue, so I'll open a ticket to track it properly before proposing
> another patch.
Have you tested without this patch but with commit c36ed72e4a59
("notmuch: speed up lastmod query")? It seems like that commit might
solve most of your performance problems by reducing the number of
messages that are re-indexed when the database changes.