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

Loading chats in Delta Chat Desktop got much slower after upgrade to core 1.156.0 #6585

Closed
link2xt opened this issue Feb 28, 2025 · 8 comments
Assignees
Labels
bug Something is not working

Comments

@link2xt
Copy link
Collaborator

link2xt commented Feb 28, 2025

I have git bisect-ed the problem and it seems to be commit deltachat/deltachat-desktop@c60aa90

Core was updated from 1.155.6 to 1.156.0. Maybe it is the core issue.

@link2xt
Copy link
Collaborator Author

link2xt commented Feb 28, 2025

In dev console I get messages "renderer/stores/MessageListStore loadChat took 1770.0999999940395".

get_messages takes more than 1.5 s is not ok.

Will try to bisect the core.

@link2xt link2xt changed the title Loading chats got much slower after upgrade to 1.156.0 Loading chats got much slower after upgrade to core 1.156.0 Feb 28, 2025
@link2xt
Copy link
Collaborator Author

link2xt commented Feb 28, 2025

Maybe slowdown is due to #6554
Anyway, bisecting it.

@link2xt link2xt transferred this issue from deltachat/deltachat-desktop Feb 28, 2025
@link2xt link2xt changed the title Loading chats got much slower after upgrade to core 1.156.0 Loading chats in Delta Chat Desktop got much slower after upgrade to core 1.156.0 Feb 28, 2025
@link2xt link2xt added the bug Something is not working label Feb 28, 2025
@link2xt
Copy link
Collaborator Author

link2xt commented Feb 28, 2025

Saved messages were added in #5606

If the issue is that get_saved_msg_id is slow, then we need an index on starred column for quick search over it.

@link2xt
Copy link
Collaborator Author

link2xt commented Feb 28, 2025

I did not bisect, but checked that this simple patch speeds up chat loading visibly:

diff --git a/deltachat-jsonrpc/src/api/types/message.rs b/deltachat-jsonrpc/src/api/types/message.rs
index a60ee15a5..4f6bda940 100644
--- a/deltachat-jsonrpc/src/api/types/message.rs
+++ b/deltachat-jsonrpc/src/api/types/message.rs
@@ -267,10 +267,7 @@ pub async fn from_msg_id(context: &Context, msg_id: MsgId) -> Result<Option<Self
                 .await?
                 .map(|id| id.to_u32()),

-            saved_message_id: message
-                .get_saved_msg_id(context)
-                .await?
-                .map(|id| id.to_u32()),
+            saved_message_id: None,

             reactions,

So we need an index (or an explicit backlink, but if a single migration adding an index works then better an index).

@link2xt link2xt self-assigned this Feb 28, 2025
@link2xt
Copy link
Collaborator Author

link2xt commented Feb 28, 2025

I manually did CREATE INDEX link2xt_starred_index ON msgs (starred) and it fixed the problem.

@link2xt
Copy link
Collaborator Author

link2xt commented Mar 1, 2025

I have made this testcase for SQLite query planner:

#!/bin/sh
{
cat <<EOF
PRAGMA foreign_keys=OFF;
BEGIN TRANSACTION;
CREATE TABLE msgs (
    id INTEGER PRIMARY KEY AUTOINCREMENT,
    txt TEXT NOT NULL,
    starred INTEGER DEFAULT 0,
    chat_id INTEGER DEFAULT 0
);
EOF

for i in $(seq 300); do
cat <<EOF
INSERT INTO msgs (txt) VALUES('foobar');
EOF
done

cat <<EOF
CREATE INDEX my_index ON msgs (starred);
COMMIT;

-- At this point query planner decides to use the index.
EXPLAIN QUERY PLAN SELECT id FROM msgs WHERE starred=? AND chat_id!=?;

ANALYZE;

-- After analyzing, query planner stops using the index and scans the whole msgs table.
EXPLAIN QUERY PLAN SELECT id FROM msgs WHERE starred=? AND chat_id!=?;

-- Now we have to resort to forcing using the index because query planner makes wrong decision.
EXPLAIN QUERY PLAN SELECT id FROM msgs INDEXED BY my_index WHERE starred=? AND chat_id!=?;
EOF
} | sqlite3

With SQLite 3.49.1 2025-02-18 13:38:58 873d4e274b4988d260ba8354a9718324a1c26187a4ab4c1cc0227c03d0f1alt1 (64-bit) this prints out:

QUERY PLAN
`--SEARCH msgs USING INDEX my_index (starred=?)
QUERY PLAN
`--SCAN msgs
QUERY PLAN
`--SEARCH msgs USING INDEX my_index (starred=?)

@link2xt
Copy link
Collaborator Author

link2xt commented Mar 1, 2025

I have reported this to SQLite forum: https://sqlite.org/forum/forumpost/49fd145748

@link2xt
Copy link
Collaborator Author

link2xt commented Mar 1, 2025

I wrote more here: https://sqlite.org/forum/forumpost/edcd4ecddb
tl;dr this starred column should have been a separate table.

@link2xt link2xt closed this as not planned Won't fix, can't repro, duplicate, stale Mar 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant