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.

Sign up for GitHub

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

Jump to bottom

IndexedDB views backend #1630

Merged

Twey merged 2 commits into linera-io:main from Twey:02-06-IndexedDB_storage

Apr 3, 2024

Contributor

Twey commented Feb 6, 2024 •

edited

Loading

Motivation

Proposal

Add an IndexedDB storage backend to linera-views.

This branch uses a patched fork of indexed_db_futures because of Alorel/rust-indexed-db#34, which will otherwise break our non-Web Wasm build.

Test Plan

A set of tests is added to CI.

Release Plan

No user-visible changes (yet).

Links

Twey force-pushed the 02-06-IndexedDB_storage branch 7 times, most recently from 41cd210 to 41dba18 Compare

February 7, 2024 18:32

MathieuDutSik reviewed

View reviewed changes

linera-core/Cargo.toml Outdated Show resolved Hide resolved

Twey force-pushed the 02-06-IndexedDB_storage branch 2 times, most recently from 87ba6b0 to 4dc6352 Compare

March 28, 2024 19:44

Twey force-pushed the 02-06-IndexedDB_storage branch from 4dc6352 to 20a491a Compare

April 2, 2024 15:43

Twey marked this pull request as ready for review

April 2, 2024 15:44

Twey force-pushed the 02-06-IndexedDB_storage branch 5 times, most recently from 607ddb5 to 054c492 Compare

April 2, 2024 17:22

Twey requested review from MathieuDutSik and ma2bd

April 2, 2024 20:03

Twey changed the title ~~IndexedDB storage backend~~ IndexedDB views backend

MathieuDutSik reviewed

View reviewed changes

Contributor

MathieuDutSik left a comment

Is there a reason for web to be a priori different from indexed_db?

.github/workflows/rust.yml

@@ @@ -37,7 +37,7 @@ permissions: @@
               jobs:
                 test:
                   runs-on: ubuntu-latest-16-cores
-                  timeout-minutes: 90
+                  timeout-minutes: 95

Contributor

MathieuDutSik Apr 3, 2024

Is that still needed, I thought that we decreased the need for such long times.

Contributor Author

Twey Apr 3, 2024 •

edited

Loading

I don't know where the 90 came from, but the +5 accounts (with a little wiggle room) for the runtime of wasm-pack test on my laptop.

linera-views/src/indexed_db.rs Show resolved Hide resolved

linera-views/src/indexed_db.rs Outdated

+              /// The number of streams for the test
+              pub const TEST_INDEX_DB_MAX_STREAM_QUERIES: usize = 10;
+              const DATABASE_NAME: &str = "linera-views";

Contributor

MathieuDutSik Apr 3, 2024

That seems inadequate. Within the framework of this library, all databases could claim to be linera-views. Why not use IndexedDB?

Contributor

ma2bd Apr 3, 2024 •

edited

Loading

I suspect this is a name provided to the (multi-application) database operated by the browser.

"linera" is perhaps a bit more precise because we sometime use a linera-views KV store directly (without views) e.g. to store certificates.

Contributor Author

Twey Apr 3, 2024 •

edited

Loading

Within IndexedDB, all databases can claim to be IndexedDB :) As the name of an IndexedDB database, this key is namespaced within IndexedDB, not within linera-views — though actually, since IndexedDB is automatically isolated between domains, this whole string is essentially arbitrary (we don't need to distinguish it from anything else).

linera-views/src/indexed_db.rs Outdated Show resolved Hide resolved

linera-views/src/indexed_db.rs

Comment on lines +145 to +155

+                      loop {
+                          let Some(key) = cursor.primary_key() else {
+                              break;
+                          };
+                          let key = js_sys::Uint8Array::new(&key);
+                          key_values.push((
+                              key.subarray(key_prefix.len() as u32, key.length()).to_vec(),
+                              js_sys::Uint8Array::new(&cursor.value()).to_vec(),
+                          ));
+                          if !cursor.continue_cursor()?.await? {
+                              break;
+                          }
+                      }

Contributor

MathieuDutSik Apr 3, 2024

It is a little bit puzzling that the find_key_values_by_prefix implementation is significantly different from the find_keys_by_prefix. Is it questions of ownership?

Contributor Author

Twey Apr 3, 2024

It's because the IndexedDB API has a convenient one-shot function for getting a range of keys, or a range of values, but not for getting both keys and values. Rather than perform one query to get the list of keys and then n queries to get a value for each key, I fell back to the slightly lower-level cursor API, which does allow getting both keys and values.

linera-views/src/indexed_db.rs Outdated Show resolved Hide resolved

linera-views/src/indexed_db.rs Outdated Show resolved Hide resolved

linera-views/src/test_utils/mod.rs Outdated Show resolved Hide resolved

Contributor Author

Twey commented Apr 3, 2024 •

edited

Loading

Is there a reason for web to be a priori different from indexed_db?

web (used across the workspace) only indicates that we're compiling for the Web target, i.e. browser APIs are available. We may wish to have different storage APIs on the Web, in the same way we support several different storage solutions on Linux. IndexedDB requires the Web, to a first approximation, but there's no need for the Web to require IndexedDB. Indeed, compiling with web but not indexeddb currently works, and gives you a linera-views that can only use the memory store :)

Ideally I think that wasm32-web or similar should be a compiler target, rather than a feature, but currently it doesn't exist. The Rust team are holding off on adding a plethora of Wasm targets until the dust settles a bit.

ma2bd reviewed

View reviewed changes

linera-views/src/indexed_db.rs Outdated Show resolved Hide resolved

linera-views/src/test_utils/mod.rs Outdated Show resolved Hide resolved

Cargo.toml Show resolved Hide resolved

linera-views/src/indexed_db.rs Outdated

+              /// The number of streams for the test
+              pub const TEST_INDEX_DB_MAX_STREAM_QUERIES: usize = 10;
+              const DATABASE_NAME: &str = "linera-views";

Contributor

ma2bd Apr 3, 2024 •

edited

Loading

I suspect this is a name provided to the (multi-application) database operated by the browser.

"linera" is perhaps a bit more precise because we sometime use a linera-views KV store directly (without views) e.g. to store certificates.

Twey force-pushed the 02-06-IndexedDB_storage branch 2 times, most recently from 5b2bf71 to 7b3ae7d Compare

April 3, 2024 14:32

Twey added 2 commits

April 3, 2024 15:41


          linera-views: add indexed_db module and tests

65295bf

Use `indexed_db_futures` to implement
`Local{Readable,Writable,Admin}KeyValueStore`.

`indexed_db_futures` is a little inconveniently low-level, but seems
to be by far the most widely-used crate for IndexedDB in Rust, so
should remain supported for some time.


          .github: test IndexedDB store

50101be

Twey force-pushed the 02-06-IndexedDB_storage branch from 7b3ae7d to 50101be Compare

April 3, 2024 14:41

Twey requested review from ma2bd and MathieuDutSik

April 3, 2024 15:24

ma2bd approved these changes

View reviewed changes

MathieuDutSik approved these changes

View reviewed changes

Contributor

MathieuDutSik left a comment

Thank you for the good work.

Twey merged commit f98ce09 into linera-io:main

5 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet