net_imap: Make untagged EXISTS more efficient for COPY/MOVE/APPEND.

The current process for generating untagged EXISTS in response
to new messages is very message-specific; the callback is
executed every single time for every single message, no matter
how many messages are part of the operation. This can be very
inefficient when a user does a batch operation, such as appending
a large number of messages to a mailbox, or copying/moving
messages between mailboxes. This was further exacerbated by
existing inefficiencies in this process, such as counting the
number of messages in the maildir every time the untagged EXISTS
callback is triggered. This meant adding a large number of messages
e.g. via COPY/MOVE resulted in a polynomial number of passes over
the contents of the maildir.

Furthermore, a large number of EXISTS updates (e.g. thousands)
can potentially fill up imap->pfd[1].

To make this more efficient, we now ensure that this callback is
only triggered once for batch operations, by doing the following:

* COPY/MOVE operations now trigger EVENT_MESSAGE_APPEND, not
  EVENT_MESSAGE_NEW. This allows net_imap to ignore all
  the APPEND events (but the NEW events), and also seems to be
  a more correct interpretation of RFC 5423 Section 4.
* net_imap now ignores EVENT_MESSAGE_APPEND.
* Add a new pseudo-event, EVENT_MESSAGE_APPEND_MULTIPLE, which is
  fired off only at the end of a batch APPEND/COPY/MOVE operation.
  This way, as far as untagged EXIST goes, we only need to send
  a single update, instead of a linear number of updates.
* net_imap now now manually fires the EVENT_MESSAGE_APPEND_MULTIPLE
  event with the UIDs that were impacted.

Overall, this yields a significant performance improvement when
appending/copying/moving lots of messages (about 10x, from some
performance benchmarks before/after this change).

Author: InterLinked1

include/mod_mail.h

@@ -53,7 +53,8 @@ enum mailbox_event_type {
 	EVENT_SERVER_METADATA_CHANGE = (1 << 19),
 	EVENT_ANNOTATION_CHANGE = (1 << 20),		/*!< Message annotation changed */
 	/* Internal events (not part of any RFC, only used by the BBS) */
-	EVENT_MAILBOX_UIDVALIDITY_CHANGE = (1 << 21)	/*!< UIDVALIDITY reset */
+	EVENT_MAILBOX_UIDVALIDITY_CHANGE = (1 << 21),	/*!< UIDVALIDITY reset */
+	EVENT_INTERNAL_MESSAGE_APPEND_MULTIPLE = (1 << 22),	/*!< Cumulative update used for efficiency in lieu of APPEND */
 };
 
 /*! \brief Get the name of a mailbox event type */

modules/mod_mail.c

@@ -213,6 +213,8 @@ const char *mailbox_event_type_name(enum mailbox_event_type type)
 			return "AnnotationChange";
 		case EVENT_MAILBOX_UIDVALIDITY_CHANGE:
 			return "UIDVALIDITYChange";
+		case EVENT_INTERNAL_MESSAGE_APPEND_MULTIPLE:
+			return "InternalMessageAppendMultiple";
 		/* No default case */
 	}
 	__builtin_unreachable();
@@ -279,7 +281,7 @@ void mailbox_dispatch_event_basic(enum mailbox_event_type type, struct bbs_node
 	mailbox_dispatch_event(&e);
 }
 
-void mailbox_notify_new_message(struct bbs_node *node, struct mailbox *mbox, const char *maildir, const char *newfile, size_t size)
+static void mailbox_notify_new_or_appended_message(struct bbs_node *node, struct mailbox *mbox, const char *maildir, const char *newfile, size_t size, enum mailbox_event_type event_type)
 {
 	struct mailbox_event e;
 	struct stat st;
@@ -308,11 +310,16 @@ void mailbox_notify_new_message(struct bbs_node *node, struct mailbox *mbox, con
 	 * for a MessageNew event, implicitly move the message from new to cur NOW, which will assign it a UID.
 	 * Otherwise, there is really no good workaround. */
 
-	mailbox_initialize_event(&e, EVENT_MESSAGE_NEW, node, mbox, maildir);
+	mailbox_initialize_event(&e, event_type, node, mbox, maildir);
 	e.msgsize = size;
 	mailbox_dispatch_event(&e);
 }
 
+void mailbox_notify_new_message(struct bbs_node *node, struct mailbox *mbox, const char *maildir, const char *newfile, size_t size)
+{
+	return mailbox_notify_new_or_appended_message(node, mbox, maildir, newfile, size, EVENT_MESSAGE_NEW);
+}
+
 void mailbox_notify_quota_exceeded(struct bbs_node *node, struct mailbox *mbox)
 {
 	struct mailbox_event e;
@@ -1710,7 +1717,7 @@ static int copy_move_untagged_exists(struct bbs_node *node, struct mailbox *mbox
 
 	safe_strncpy(maildir, newpath, sizeof(maildir));
 	maildir_extract_from_filename(maildir);
-	mailbox_notify_new_message(node, mbox, maildir, newpath, size);
+	mailbox_notify_new_or_appended_message(node, mbox, maildir, newpath, size, EVENT_MESSAGE_APPEND);
 	return 0;
 }
 

modules/mod_mail_events.c

@@ -206,6 +206,12 @@ static void log_cb(struct mailbox_event *e)
 /*! \brief Callback for all mailbox events */
 static void mbox_event_callback(struct mailbox_event *event)
 {
+	if (event->type == EVENT_INTERNAL_MESSAGE_APPEND_MULTIPLE) {
+		/* We already log everything for each individual message,
+		 * we don't care about the batch event. */
+		return;
+	}
+
 	/* Serialize logging for events, so events aren't logged partially interleaved with each other.
 	 * Obviously, this may reduce performance. */
 	bbs_mutex_lock(&loglock);

nets/net_imap.c

@@ -808,6 +808,10 @@ static void imap_mbox_watcher(struct mailbox_event *event)
 
 	switch (event->type) {
 		case EVENT_MESSAGE_APPEND:
+			/* We ignore this event as far as sending untagged EXISTS, and just use EVENT_INTERNAL_MESSAGE_APPEND_MULTIPLE
+			 * to send a single update at the end of a batch operation. */
+			break;
+		case EVENT_INTERNAL_MESSAGE_APPEND_MULTIPLE: /* Pseudo event used for batch updates */
 		case EVENT_MESSAGE_NEW:
 			send_untagged_exists(event->node, event->mbox, event->maildir);
 			break;
@@ -2044,6 +2048,19 @@ static int handle_fetch(struct imap_session *imap, char *s, int usinguid)
 	return handle_fetch_full(imap, s, usinguid, 1);
 }
 
+static void copy_move_append_multiupdate(struct imap_session *imap, const char *dstmaildir, unsigned int *newuids, int lengths)
+{
+	/* Batch an update for all new UIDs for the target mailbox and dispatch an EVENT_INTERNAL_MESSAGE_APPEND_MULTIPLE event.
+	 * mailbox_notify_new_or_appended_message has already been called at this point,
+	 * so we don't need to process any individual messages. */
+
+	/* We don't actually need these; in send_untagged_exists, we calculate how many messages are in the maildir. */
+	UNUSED(newuids);
+	UNUSED(lengths);
+
+	mailbox_dispatch_event_basic(EVENT_INTERNAL_MESSAGE_APPEND_MULTIPLE, imap->node, imap->mbox, dstmaildir);
+}
+
 static int handle_copy(struct imap_session *imap, const char *sequences, const char *newbox, int usinguid)
 {
 	struct dirent *entry, **entries;
@@ -2114,6 +2131,9 @@ static int handle_copy(struct imap_session *imap, const char *sequences, const c
 	if (olduids || newuids) {
 		olduidstr = gen_uintlist(olduids, lengths);
 		newuidstr = gen_uintlist(newuids, lengths);
+
+		copy_move_append_multiupdate(imap, newboxdir, newuids, lengths);
+
 		free_if(olduids);
 		free_if(newuids);
 	}
@@ -2122,8 +2142,6 @@ static int handle_copy(struct imap_session *imap, const char *sequences, const c
 	} else if (!numcopies && quotaleft <= 0) {
 		imap_reply(imap, "NO [OVERQUOTA] Insufficient quota remaining");
 	} else {
-		/* maildir_copy_msg_filename sends the untagged EXISTS for the new message in the destination mailbox.
-		 * No untagged EXISTS should be sent to the client running the command. */
 		if (IMAP_HAS_ACL(imap->acl, IMAP_ACL_READ)) {
 			imap_reply(imap, "OK [COPYUID %u %s %s] COPY completed", uidvalidity, S_IF(olduidstr), S_IF(newuidstr));
 		} else {
@@ -2223,14 +2241,14 @@ static int handle_move(struct imap_session *imap, const char *sequences, const c
 	if (olduids || newuids) {
 		olduidstr = gen_uintlist(olduids, lengths);
 		newuidstr = gen_uintlist(newuids, lengths);
+
+		copy_move_append_multiupdate(imap, newboxdir, newuids, lengths);
+
 		free_if(olduids);
 		free_if(newuids);
 	}
 
-	/* maildir_move_msg_filename sends the untagged EXISTS for the new message in the destination mailbox.
-	 * No untagged EXISTS should be sent to the client running the command.
-	 *
-	 * The untagged EXPUNGE responses are sent at this point, and since untagged EXPUNGES can't be sent
+	/* The untagged EXPUNGE responses are sent at this point, and since untagged EXPUNGES can't be sent
 	 * while a command is not in progress, we need to flush this out BEFORE finalizing the response with an OK. */
 	if (expunged) {
 		imap->highestmodseq = maildir_indicate_expunged(EVENT_MESSAGE_EXPUNGE, imap->node, imap->mbox, imap->curdir, expunged, expungedseqs, exp_lengths, 0);
@@ -2636,7 +2654,7 @@ static int handle_append(struct imap_session *imap, char *s)
 		 * that isn't the spirit of an "atomic" operation... */
 	}
 
-	/* We can return directly here because a isn't used when imap->client != NULL and therefore doesn't need to be freed */
+	/* We can return directly here because a uintlist isn't used when imap->client != NULL and therefore doesn't need to be freed */
 	if (imap->client) {
 		/* Send empty line to denote end of APPEND/MULTIAPPEND */
 		ssize_t res = bbs_write(imap->client->client.wfd, "\r\n", 2);
@@ -2652,6 +2670,8 @@ static int handle_append(struct imap_session *imap, char *s)
 		return 0;
 	}
 
+	copy_move_append_multiupdate(imap, appenddir, a, lengths);
+
 	if (append_response(imap, imap->acl, a, appends, uidvalidity, uidnext)) {
 		goto cleanup2;
 	}
@@ -3078,6 +3098,10 @@ static int handle_remote_move(struct imap_session *imap, char *dest, const char
 			 * has not necessarily yet come. */
 		}
 
+		if (!destclient) {
+			copy_move_append_multiupdate(imap, traversalstack.dir, a, alengths);
+		}
+
 		/* XXX If destclient, in theory if the remote server supports UIDPLUS, we could keep track of the dest UID
 		 * for each message, but we definitely can't do a passthrough of the response since there could be many messages involved. */
 		if (!destclient && usinguid && IMAP_HAS_ACL(imap->acl, IMAP_ACL_READ)) {