Add members to chats in a single sql transation (#3780)

This esp. speeds up receive_imf a bit when we recreate the member list (recreate_member_list == true).

It's a preparation for https://github.com/deltachat/deltachat-core-rust/issues/3768, which will be a one-line-fix, but recreate the member list more often, so that we want to optimize this case a bit.

It also adds a benchmark for this case. It's not that easy to make the benchmark non-flaky, but by closing all other programs and locking the CPU to 1.5GHz it worked. It is consistently a few percent faster than ./without-optim:

```
Receive messages/Receive 100 Chat-Group-Member-{Added|Removed} messages                                                                            
                        time:   [52.257 ms 52.569 ms 52.941 ms]
                        change: [-3.5301% -2.6181% -1.6697%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  4 (4.00%) high mild
  3 (3.00%) high severe
```

CHANGELOG.md

@@ -4,6 +4,7 @@
 
 ### Changes
 - Refactor: Remove the remaining AsRef<str> #3669
+- Small speedup #3780
 
 ### API-Changes
 - Add Python API to send reactions #3762

benches/receive_emails.rs

@@ -38,11 +38,64 @@ Hello {i}",
     context
 }
 
+/// Receive 100 emails that remove charlie@example.com and add
+/// him back
+async fn recv_groupmembership_emails(context: Context) -> Context {
+    for i in 0..50 {
+        let imf_raw = format!(
+            "Subject: Benchmark
+Message-ID: Gr.OssSYnOFkhR.{i}@testrun.org
+Date: Sat, 07 Dec 2019 19:00:27 +0000
+To: alice@example.com, b@example.com, c@example.com, d@example.com, e@example.com, f@example.com
+From: sender@testrun.org
+Chat-Version: 1.0
+Chat-Disposition-Notification-To: sender@testrun.org
+Chat-User-Avatar: 0
+Chat-Group-Member-Added: charlie@example.com
+In-Reply-To: Gr.OssSYnOFkhR.{i_dec}@testrun.org
+MIME-Version: 1.0
+
+Content-Type: text/plain; charset=utf-8; format=flowed; delsp=no
+
+Hello {i}",
+            i = i,
+            i_dec = i - 1,
+        );
+        receive_imf(&context, black_box(imf_raw.as_bytes()), false)
+            .await
+            .unwrap();
+
+        let imf_raw = format!(
+            "Subject: Benchmark
+Message-ID: Gr.OssSYnOFkhR.{i}@testrun.org
+Date: Sat, 07 Dec 2019 19:00:27 +0000
+To: alice@example.com, b@example.com, c@example.com, d@example.com, e@example.com, f@example.com
+From: sender@testrun.org
+Chat-Version: 1.0
+Chat-Disposition-Notification-To: sender@testrun.org
+Chat-User-Avatar: 0
+Chat-Group-Member-Removed: charlie@example.com
+In-Reply-To: Gr.OssSYnOFkhR.{i_dec}@testrun.org
+MIME-Version: 1.0
+
+Content-Type: text/plain; charset=utf-8; format=flowed; delsp=no
+
+Hello {i}",
+            i = i,
+            i_dec = i - 1,
+        );
+        receive_imf(&context, black_box(imf_raw.as_bytes()), false)
+            .await
+            .unwrap();
+    }
+    context
+}
+
 async fn create_context() -> Context {
     let dir = tempdir().unwrap();
     let dbfile = dir.path().join("db.sqlite");
     let id = 100;
-    let context = Context::new(&dbfile, id, Events::new(), StockStrings::new())
+    let context = Context::new(dbfile.as_path(), id, Events::new(), StockStrings::new())
         .await
         .unwrap();
 
@@ -52,7 +105,7 @@ async fn create_context() -> Context {
 
     if backup.exists() {
         println!("Importing backup");
-        imex(&context, ImexMode::ImportBackup, &backup, None)
+        imex(&context, ImexMode::ImportBackup, backup.as_path(), None)
             .await
             .unwrap();
     }
@@ -83,6 +136,20 @@ fn criterion_benchmark(c: &mut Criterion) {
             }
         });
     });
+    group.bench_function(
+        "Receive 100 Chat-Group-Member-{Added|Removed} messages",
+        |b| {
+            let rt = tokio::runtime::Runtime::new().unwrap();
+            let context = rt.block_on(create_context());
+
+            b.to_async(&rt).iter(|| {
+                let ctx = context.clone();
+                async move {
+                    recv_groupmembership_emails(black_box(ctx)).await;
+                }
+            });
+        },
+    );
     group.finish();
 }
 

src/chat.rs

@@ -2563,7 +2563,7 @@ pub async fn create_group_chat(
 
     let chat_id = ChatId::new(u32::try_from(row_id)?);
     if !is_contact_in_chat(context, chat_id, ContactId::SELF).await? {
-        add_to_chat_contacts_table(context, chat_id, ContactId::SELF).await?;
+        add_to_chat_contacts_table(context, chat_id, &[ContactId::SELF]).await?;
     }
 
     context.emit_msgs_changed_without_ids();
@@ -2624,19 +2624,25 @@ pub async fn create_broadcast_list(context: &Context) -> Result<ChatId> {
     Ok(chat_id)
 }
 
-/// Adds a contact to the `chats_contacts` table.
+/// Adds contacts to the `chats_contacts` table.
 pub(crate) async fn add_to_chat_contacts_table(
     context: &Context,
     chat_id: ChatId,
-    contact_id: ContactId,
+    contact_ids: &[ContactId],
 ) -> Result<()> {
     context
         .sql
-        .execute(
+        .transaction(move |transaction| {
-            "INSERT INTO chats_contacts (chat_id, contact_id) VALUES(?, ?)",
+            for contact_id in contact_ids {
-            paramsv![chat_id, contact_id],
+                transaction.execute(
-        )
+                    "INSERT OR IGNORE INTO chats_contacts (chat_id, contact_id) VALUES(?, ?)",
+                    paramsv![chat_id, contact_id],
+                )?;
+            }
+            Ok(())
+        })
         .await?;
+
     Ok(())
 }
 
@@ -2738,7 +2744,7 @@ pub(crate) async fn add_contact_to_chat_ex(
         if is_contact_in_chat(context, chat_id, contact_id).await? {
             return Ok(false);
         }
-        add_to_chat_contacts_table(context, chat_id, contact_id).await?;
+        add_to_chat_contacts_table(context, chat_id, &[contact_id]).await?;
     }
     if chat.typ == Chattype::Group && chat.is_promoted() {
         msg.viewtype = Viewtype::Text;

src/peerstate.rs

@@ -4,7 +4,7 @@ use std::collections::HashSet;
 use std::fmt;
 
 use crate::aheader::{Aheader, EncryptPreference};
-use crate::chat::{self, is_contact_in_chat, Chat};
+use crate::chat::{self, Chat};
 use crate::chatlist::Chatlist;
 use crate::constants::Chattype;
 use crate::contact::{addr_cmp, Contact, Origin};
@@ -523,9 +523,7 @@ impl Peerstate {
                     let (new_contact_id, _) =
                         Contact::add_or_lookup(context, "", new_addr, Origin::IncomingUnknownFrom)
                             .await?;
-                    if !is_contact_in_chat(context, *chat_id, new_contact_id).await? {
+                    chat::add_to_chat_contacts_table(context, *chat_id, &[new_contact_id]).await?;
-                        chat::add_to_chat_contacts_table(context, *chat_id, new_contact_id).await?;
-                    }
 
                     context.emit_event(EventType::ChatModified(*chat_id));
                 }

src/receive_imf.rs

@@ -1530,19 +1530,13 @@ async fn create_or_lookup_group(
         chat_id_blocked = create_blocked;
 
         // Create initial member list.
-        chat::add_to_chat_contacts_table(context, new_chat_id, ContactId::SELF).await?;
+        let mut members = vec![ContactId::SELF];
-        if !from_id.is_special() && !chat::is_contact_in_chat(context, new_chat_id, from_id).await?
+        if !from_id.is_special() {
-        {
+            members.push(from_id);
-            chat::add_to_chat_contacts_table(context, new_chat_id, from_id).await?;
-        }
-        for &to_id in to_ids.iter() {
-            info!(context, "adding to={:?} to chat id={}", to_id, new_chat_id);
-            if to_id != ContactId::SELF
-                && !chat::is_contact_in_chat(context, new_chat_id, to_id).await?
-            {
-                chat::add_to_chat_contacts_table(context, new_chat_id, to_id).await?;
-            }
         }
+        members.extend(to_ids);
+        members.dedup();
+        chat::add_to_chat_contacts_table(context, new_chat_id, &members).await?;
 
         // once, we have protected-chats explained in UI, we can uncomment the following lines.
         // ("verified groups" did not add a message anyway)
@@ -1698,6 +1692,7 @@ async fn apply_group_changes(
             .update_timestamp(context, Param::MemberListTimestamp, sent_timestamp)
             .await?
         {
+            let mut members_to_add = vec![];
             if removed_id.is_some()
                 || !chat::is_contact_in_chat(context, chat_id, ContactId::SELF).await?
             {
@@ -1712,26 +1707,23 @@ async fn apply_group_changes(
                     )
                     .await?;
 
-                if removed_id != Some(ContactId::SELF) {
+                members_to_add.push(ContactId::SELF);
-                    chat::add_to_chat_contacts_table(context, chat_id, ContactId::SELF).await?;
-                }
             }
-            if !from_id.is_special()
+
-                && from_id != ContactId::SELF
+            if !from_id.is_special() {
-                && !chat::is_contact_in_chat(context, chat_id, from_id).await?
+                members_to_add.push(from_id);
-                && removed_id != Some(from_id)
-            {
-                chat::add_to_chat_contacts_table(context, chat_id, from_id).await?;
             }
-            for &to_id in to_ids.iter() {
+            members_to_add.extend(to_ids);
-                if to_id != ContactId::SELF
+            if let Some(removed_id) = removed_id {
-                    && !chat::is_contact_in_chat(context, chat_id, to_id).await?
+                members_to_add.retain(|id| *id != removed_id);
-                    && removed_id != Some(to_id)
-                {
-                    info!(context, "adding to={:?} to chat id={}", to_id, chat_id);
-                    chat::add_to_chat_contacts_table(context, chat_id, to_id).await?;
-                }
             }
+            members_to_add.dedup();
+
+            info!(
+                context,
+                "adding {:?} to chat id={}", members_to_add, chat_id
+            );
+            chat::add_to_chat_contacts_table(context, chat_id, &members_to_add).await?;
             send_event_chat_modified = true;
         }
     }
@@ -1883,7 +1875,7 @@ async fn create_or_lookup_mailinglist(
             )
         })?;
 
-        chat::add_to_chat_contacts_table(context, chat_id, ContactId::SELF).await?;
+        chat::add_to_chat_contacts_table(context, chat_id, &[ContactId::SELF]).await?;
         Ok(Some((chat_id, Blocked::Request)))
     } else {
         info!(context, "creating list forbidden by caller");
@@ -2013,9 +2005,7 @@ async fn create_adhoc_group(
         None,
     )
     .await?;
-    for &member_id in member_ids.iter() {
+    chat::add_to_chat_contacts_table(context, new_chat_id, member_ids).await?;
-        chat::add_to_chat_contacts_table(context, new_chat_id, member_id).await?;
-    }
 
     context.emit_event(EventType::ChatModified(new_chat_id));
 
@@ -5160,13 +5150,10 @@ Reply from different address
         chat::add_to_chat_contacts_table(
             &bob,
             group_id,
-            bob.add_or_lookup_contact(&alice1).await.id,
+            &[
-        )
+                bob.add_or_lookup_contact(&alice1).await.id,
-        .await?;
+                Contact::create(&bob, "", "charlie@example.org").await?,
-        chat::add_to_chat_contacts_table(
+            ],
-            &bob,
-            group_id,
-            Contact::create(&bob, "", "charlie@example.org").await?,
         )
         .await?;
 
@@ -5247,7 +5234,7 @@ Reply from different address
         chat::add_to_chat_contacts_table(
             &bob,
             group_id,
-            bob.add_or_lookup_contact(&alice).await.id,
+            &[bob.add_or_lookup_contact(&alice).await.id],
         )
         .await?;
 

src/securejoin/bob.rs

@@ -60,7 +60,7 @@ pub(super) async fn start_protocol(context: &Context, invite: QrInvite) -> Resul
             // TODO: how does this group become usable?
             let group_chat_id = state.joining_chat_id(context).await?;
             if !is_contact_in_chat(context, group_chat_id, invite.contact_id()).await? {
-                chat::add_to_chat_contacts_table(context, group_chat_id, invite.contact_id())
+                chat::add_to_chat_contacts_table(context, group_chat_id, &[invite.contact_id()])
                     .await?;
             }
             let msg = stock_str::secure_join_started(context, invite.contact_id()).await;

src/sql.rs

@@ -432,7 +432,7 @@ impl Sql {
     pub async fn transaction<G, H>(&self, callback: G) -> Result<H>
     where
         H: Send + 'static,
-        G: Send + 'static + FnOnce(&mut rusqlite::Transaction<'_>) -> Result<H>,
+        G: Send + FnOnce(&mut rusqlite::Transaction<'_>) -> Result<H>,
     {
         let mut conn = self.get_conn().await?;
         tokio::task::block_in_place(move || {

src/sql/migrations.rs

@@ -649,6 +649,17 @@ CREATE INDEX smtp_messageid ON imap(rfc724_mid);
         )
         .await?;
     }
+    if dbversion < 95 {
+        sql.execute_migration(
+            "CREATE TABLE new_chats_contacts (chat_id INTEGER, contact_id INTEGER, UNIQUE(chat_id, contact_id));\
+            INSERT OR IGNORE INTO new_chats_contacts SELECT * FROM chats_contacts;\
+            DROP TABLE chats_contacts;\
+            ALTER TABLE new_chats_contacts RENAME TO chats_contacts;\
+            CREATE INDEX chats_contacts_index1 ON chats_contacts (chat_id);\
+            CREATE INDEX chats_contacts_index2 ON chats_contacts (contact_id);",
+            95
+        ).await?;
+    }
 
     let new_version = sql
         .get_raw_config_int(VERSION_CFG)