Skip to content

Conversation

@Hywan
Copy link
Member

@Hywan Hywan commented Nov 7, 2025

Initially, we were using deadpool-sqlite, that is also using rusqlite as the SQLite interface. However, in the implementation of deadpool::managed::Manager, when recycling an object (i.e. an SQLite connection), an SQL query is run to detect whether the connection is still alive. It creates performance issues:

  1. It runs a prepared SQL query, which has a non-negligle cost. Imagine each connection is used to run on average one query; when recycled, a second query was constantly run. Even if it's a simple query, it requires to prepare a statement, to run and to query it.
  2. The SQL query was run in a blocking task. Indeed, deadpool_runtime::spawn_blocking is used (via deadpool_sync::SyncWrapper::interact), which includes using a blocking thread (they are limited), acquiring a lock etc. All this has more performance cost.

Measures have shown it is a performance bottleneck for us, especially on Android. Why specifically on Android and not other systems? This is still unclear at the time of writing (2025-11-11), despite having spent several days digging and trying to find an answer to this question (kudos to @jmartinesp who didn't count its hours on this…).

We have tried to use another approach to test the aliveness of the connections without running queries. It has involved patching rusqlite to add more bindings to SQLite, and patching deadpool itself, but without any successful results.

Finally, we have started questioning the reason of this test: why testing whether the connection was still alive? After all, there is no reason a connection should die in our case:

  • all connections are local,
  • all interactions are behind WAL, which is local only,
  • even if for an unknown reason the connection died, using it next time would create an error… exactly what would happen when recycling the connection.

Consequently, we have created a new implementation of deadpool for rusqlite that doesn't test the aliveness of the connections when recycled. We assume they are all alive.

This implementation is, at the time of writing (2025-11-11):

  • 3.5 times faster on Android than deadpool-sqlite, removing the lock and thread contention entirely,
  • 2 times faster on iOS.

@codecov
Copy link

codecov bot commented Nov 7, 2025

Codecov Report

❌ Patch coverage is 73.80952% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.61%. Comparing base (3b14184) to head (eaec1db).
⚠️ Report is 13 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
crates/matrix-sdk-sqlite/src/crypto_store.rs 0.00% 0 Missing and 4 partials ⚠️
crates/matrix-sdk-sqlite/src/state_store.rs 42.85% 0 Missing and 4 partials ⚠️
crates/matrix-sdk-sqlite/src/connection.rs 84.21% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5841   +/-   ##
=======================================
  Coverage   88.61%   88.61%           
=======================================
  Files         361      362    +1     
  Lines      102269   102292   +23     
  Branches   102269   102292   +23     
=======================================
+ Hits        90627    90648   +21     
- Misses       7420     7422    +2     
  Partials     4222     4222           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 7, 2025

CodSpeed Performance Report

Merging #5841 will not alter performance

Comparing Hywan:feat-sqlite-custom-pool-manager (eaec1db) with main (fff270d)

Summary

✅ 50 untouched

@Hywan Hywan force-pushed the feat-sqlite-custom-pool-manager branch from 5c889a8 to 0a3a594 Compare November 11, 2025 09:41
@Hywan Hywan marked this pull request as ready for review November 11, 2025 09:45
@Hywan Hywan requested a review from a team as a code owner November 11, 2025 09:45
@Hywan Hywan requested review from jmartinesp, poljar and stefanceriu and removed request for a team and poljar November 11, 2025 09:45
This patch replaces `deadpool-sqlite` by our own implementation in
`crate::connection`. It still uses `deadpool` but the object manager has
a different implementation.
This patch explains why we create our own implementation of `deadpool`
for `rusqlite`.
This patch removes the `connection::Config` type. It was “inspired”
from `deadpool_sqlite`, but we can clearly remove it by using our own
`SqliteStoreConfig` type. It simplifies the way we open a database.
@Hywan Hywan force-pushed the feat-sqlite-custom-pool-manager branch from 0a3a594 to eaec1db Compare November 11, 2025 09:49
Copy link
Contributor

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been using it for 4 days now with no issues and outstanding performance, so it definitely LGTM!

@Hywan Hywan merged commit 610f82a into matrix-org:main Nov 11, 2025
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants