Add wasm32-unknown-unknown compilation target support for matrix_sdk_sqlite#6329
Add wasm32-unknown-unknown compilation target support for matrix_sdk_sqlite#6329NoManColonies wants to merge 32 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6329 +/- ##
==========================================
+ Coverage 89.82% 89.83% +0.01%
==========================================
Files 378 379 +1
Lines 104337 104330 -7
Branches 104337 104330 -7
==========================================
+ Hits 93719 93729 +10
+ Misses 7016 7007 -9
+ Partials 3602 3594 -8 ☔ View full report in Codecov by Sentry. |
Merging this PR will degrade performance by 49.4%
Performance Changes
Comparing |
There was a problem hiding this comment.
Thanks very much for submitting this. We're quite excited by it, as it opens the door to things like being able to use the wasm bindings for matrix-sdk-crypto in node.js (matrix-org/matrix-sdk-crypto-wasm#168), and maybe even dropping support for indexeddb altogether.
Some questions, other than those below:
-
How were you able to test/build this? I tried things like
cargo check -p matrix-sdk-sqlite --target wasm32-unknown-unknownandwasm-pack test --chrome --headless crates/matrix-sdk-sqlite, and got build errors.Would you be able to add support to
cargo xtask cifor checking and testingmatrix-sdk-sqlitewith the wasm target? I think you'll need to add a new entry toWasmFeatureSet. -
Would you be able to break the changes down into smaller commits, to make them easier to review?
|
@NoManColonies I assume you're still working on this. Once it's ready for review, please rebase your changes on |
5e409c1 to
bf489aa
Compare
Thank you very much for taking your time to review this PR and apologize for the late reply.
I initially tested the prototype in a temporary downstream project and I seem to have forgotten to properly configure the feature requirements in the upstream crate, my apologies. You should be able to build and test with the following commands:
I’ve added tests as requested, but current VFS backend (OPFS) require dedicated worker context and is therefore not supported in a
Apologies for the large initial commit, it was initially composed of many small commit that didn’t compile. I saw a note in CONTRIBUTING.md that requires all commits to compile successfully, and all initial changes were required in order to compile under WASM targets. If you'd prefer, I can break the initial commit down into a smaller one. Since your previous comments, I have made the following changes:
If you have additional requests, please let know. |
chore: address breaking changes from rusqlite feat: support alternative v/fs for wasm environment feat: support non-send future for wasm environment fix: solve incorrect error transformation feat: WIP support connection pool in wasm environment fix: use full-path method call for wasm environment + other fixes for wasm fix: more wasm related fixes feat: use RefCell to simulate interior-mutability in wasm environment fix: use implicit borrow instead of explicit call to `as_ref()` fix: more wasm related fixes fix: resolve ambiguity between lock guard and ref cell implicit reference + more wasm fixes fix: resolve wasm related lifetime issues chore: turn on serde feature for web time when compiled as wasm32 target chore: ran cargo check to verify Cargo.lock file chore: fix comment formatting
…g conditional compilation
richvdh
left a comment
There was a problem hiding this comment.
Thanks very much for the updates. I've taken a more detailed look. Generally it looks great, but I have some comments.
| //! Similar to the one implemented in `crate::connection::default`, | ||
| //! we do not implement connection recycling here. Mostly due to | ||
| //! [`managed::Manager`] trait expecting a future output with `Send` | ||
| //! bound which is not available in WASM environment. |
There was a problem hiding this comment.
I don't really understand this comment. What does "do not implement connection recycling" mean? In what sense does Manager expect a future with Send?
There was a problem hiding this comment.
(I still don't understand this, but will return to it once we've resolved the other thread about recycle.
| // SQLite WASM test suites has to be ran in release mode due to | ||
| // a harmless debug assertion when closing database. | ||
| // | ||
| // Ref: https://github.com/Spxg/sqlite-wasm-rs/blob/master/crates/sqlite-wasm-vfs/src/sahpool.rs#L672 |
There was a problem hiding this comment.
why does this happen? The assertion is "close without open", but doesn't the database file get opened?
There was a problem hiding this comment.
I’m not entirely sure why this happens. I don’t have deep insight into SQLite internals or the VFS implementation, but testing in a real browser environment seems to work as expected. Creating and opening a database in OPFS behaves correctly.
I’m not sure why this assertion ("close without open") is being triggered in this case. It may be worth checking with the author of https://github.com/Spxg/sqlite-wasm-rs for more insight.
|
@NoManColonies are you still working on this? |
257fa8e to
a7cde3b
Compare
@richvdh Apologies for the delay. I got a bit caught up with some IRL stuff. I've made the following changes as requested:
|
|
@NoManColonies no need to apologise for the delay: I am happy to know you are still working on this! However, I see you have force-pushed since my previous review, which means there is no way for me to see what has changed. As https://github.com/matrix-org/matrix-rust-sdk/blob/main/CONTRIBUTING.md says: "Do not force push after a review has started." In order for me to see what has changed since 4bd5f07, please:
|
…_up_fs multiple times
Apologies for that. I’ve reset the branch back to |
|
@NoManColonies sorry for taking a long time to get back to this. It's on my to-do list, I promise I'll take a look soon. |
... for some value of "soon" ... |
richvdh
left a comment
There was a problem hiding this comment.
@NoManColonies thanks for making all these changes; I think it is going in the right direction.
I've made a few requests for changes, but, to be honest, I'm finding it a bit hard to keep track of and I think it might be a good idea to make some separate PRs which we can land separately, to reduce the size of this one.
In particular, we could start with PRs for the following:
- Change
connection::Manager::new()to callcreate_dir_all, with the corresponding changes tobuild_pool_of_connections - Change
crypto_store::tests::get_test_dbto take the database bytes, rather than the filename
| } | ||
| } | ||
|
|
||
| #[cfg(all(target_family = "wasm", target_os = "unknown"))] |
There was a problem hiding this comment.
we can omit this, given the whole module is only built for wasm. Likewise below.
| // We cannot implement connection recycling | ||
| // at the moment, due to | ||
| // `managed::Manager::recycle` expecting | ||
| // a future with `Send` bound which is not | ||
| // available in WASM environments. |
There was a problem hiding this comment.
I'm still not entirely understanding this. What would be involved in implementing recycling? You mention that we can't call ConnectionWrapper::interact, but why would we want do so?
| /// Wrapper object for providing interior mutability similar to [`SyncWrapper`] | ||
| /// without `Send` requirement. |
There was a problem hiding this comment.
| /// Wrapper object for providing interior mutability similar to [`SyncWrapper`] | |
| /// without `Send` requirement. | |
| /// Wrapper object, providing interior mutability similar to [`SyncWrapper`], | |
| /// but without requiring that the wrapped type implements `Send`. |
| } | ||
|
|
||
| /// Configure VFS name using provided path. | ||
| pub fn get_vfs_name(path: &Path) -> String { |
There was a problem hiding this comment.
This can be private now
| pub fn get_vfs_name(path: &Path) -> String { | |
| fn get_vfs_name(path: &Path) -> String { |
| } | ||
| } | ||
|
|
||
| /// Configure VFS name using provided path. |
There was a problem hiding this comment.
| /// Configure VFS name using provided path. | |
| /// Construct a suitable VFS name, representing the given native path. |
|
|
||
| use deadpool::managed::{self, Metrics}; | ||
| use rusqlite::OpenFlags; | ||
| use sqlite_wasm_vfs::sahpool::{OpfsSAHPoolCfgBuilder, OpfsSAHPoolUtil, install}; |
There was a problem hiding this comment.
suggest giving install a better name:
| use sqlite_wasm_vfs::sahpool::{OpfsSAHPoolCfgBuilder, OpfsSAHPoolUtil, install}; | |
| use sqlite_wasm_vfs::sahpool::{OpfsSAHPoolCfgBuilder, OpfsSAHPoolUtil, install as install_opfs_sahpool}; |
(or just refer to it as sqlite_wasm_vfs::sahpool::install)
| /// tool. | ||
| /// | ||
| /// Subsequence call to this function will simply return the management tool | ||
| /// without installing vfs. |
There was a problem hiding this comment.
| /// without installing vfs. | |
| /// Register a [SQLite VFS], using [OPFS] and the provided path. | |
| /// | |
| /// Returns a management utility (primarily useful for testing). | |
| /// | |
| /// Subsequent calls to this function will simply return the management tool | |
| /// without re-registering the VFS. | |
| /// | |
| /// [SQLite VFS]: https://sqlite.org/vfs.html | |
| /// [OPFS]: https://developer.mozilla.org/en-US/docs/Web/API/File_System_API/Origin_private_file_system |
Also, maybe the function should be called register_vfs or install_vfs ?
| .vfs_name(&get_vfs_name(path)) | ||
| .directory(path.to_string_lossy().as_ref()) | ||
| .build(); | ||
| // Avoid global installation, due to being harder to test. |
There was a problem hiding this comment.
| // Avoid global installation, due to being harder to test. | |
| // Avoid installing as the default VFS, due to being harder to test. |
| /// | ||
| /// Subsequence call to this function will simply return the management tool | ||
| /// without installing vfs. | ||
| pub async fn setup_vfs(path: &Path) -> Result<OpfsSAHPoolUtil, OpenStoreError> { |
There was a problem hiding this comment.
maybe this should return OpfsSAHError ?
| // Load test database source during compile-time, since we do not have | ||
| // access to file system during runtime for WASM targets. | ||
| let TestDb { _dir: _, database } = get_test_db( | ||
| include_bytes!("../../../testing/data/storage/matrix-sdk-crypto.wasm.db"), |
There was a problem hiding this comment.
these files probably should not be called *.wasm.db now that they are used for both wasm and native builds.
| - Support compiling to `wasm32-unknown-unknown` target + bump [rusqlite](https://github.com/rusqlite/rusqlite/releases/tag/v0.38.0) version to `0.38` | ||
| ([#6329](https://github.com/matrix-org/matrix-rust-sdk/pull/6329)) |
There was a problem hiding this comment.
We switched to a towncrier-based setup for changelogs. Sorry for the inconvenience but please move this to the changelog.d folder as a Towncrier fragment.
Before starting, I apologize for any inconvenience caused by my non-native English.
This PR adds support for compiling
matrix_sdk_sqliteto thewasm32-unknown-unknowntarget, enabling use in browser and similar WASM environments.Currently,
matrix_sdkin WASM is limited to:This means persistent event/media caching is not available without a custom implementation.
This PR introduces SQLite-backed persistence for WASM as an additional store option.
Summary of changes
0.38(addswasm32-unknown-unknownsupport viasqlite-wasm-rs)cachefeature now required for prepared/cached statementsfallible_uintfeature required for unsigned integer castingwasm32-wasi-vfsfeature for WASM targets to enable a browser-compatible VFSweb-timedependency for media retention policy serializationfsfeature anddeadpool-syncbehind non-WASM targetsSendwithSendOutsideWasmwhere required for non-Send WASM environments#[cfg_attr(...)]instead of unconditional#[async_trait]for target-specific behaviorSyncWrapper<T>withRefCell<T>in the WASMdeadpooladapterSyncWrapper<T>requiresSendboundRefCell<T>provides equivalent interior mutability withoutSendboundNotes for reviewers
jsfeature is needed in order to compile to WASM targettarget_family = "wasm"CHANGELOG.mdfilesSigned-off-by: Geng Kongpop [email protected]