Skip to content
This repository has been archived by the owner on Sep 21, 2024. It is now read-only.

feat: prototype implementation of SqliteStorage #549

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

jsantell
Copy link
Contributor

@jsantell jsantell commented Aug 2, 2023

MVP of a sqlite-backed Storage provider.

  • Passes tests. Run cargo test --features sqlite. Test timing increased from 27s to 42s (for me).
  • Uses the message_channel functionality from noosphere_ns. Moved this into noosphere_core as general utility, but as storage is a core dependency, no go. Copied over for now. Maybe a no-noosphere-dependency crate like noosphere-utils for something like this.
  • Each SqliteStorage creates a background thread (well, technically SqliteStore does this) and SqliteClient reused across all shared SqliteStore instances. We could key the background thread by (absolute) path such that multiple invocations of Storage also reuse the same worker thread.
  • TBD on next steps here

Copy link
Collaborator

@cdata cdata left a comment

Choose a reason for hiding this comment

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

A few high-level thoughts to get us started on this...

Each SqliteStorage creates a background thread (well, technically SqliteStore does this) and SqliteClient reused across all shared SqliteStore instances. We could key the background thread by (absolute) path such that multiple invocations of Storage also reuse the same worker thread.

  • We probably have to be careful to use actual threads here unless SQLite bindings give us guarantees about blocking behavior. If SQLite blocks on a thread where tokio doesn't expect blocking work to happen, we could end up with scheduling deadlocks. Remember: SQLite cannot cooperate with the tokio runtime where thread pool management / scheduling is concerned.
  • I'm not sure that handling an internal thread for reads/writes is the right approach here. It seems like we can just as easily assume "do work on current thread," and then wrap the storage in a generic "operations occur on dedicated thread" thing.

rust/noosphere-ns/src/dht/channel.rs Outdated Show resolved Hide resolved
@cdata
Copy link
Collaborator

cdata commented Aug 26, 2023

If we have a "blocking" storage implementation, the level of abstraction where I would like to deal with that is outside of the storage implementation itself. That would give us more control over scheduling, and we could circumstantially decide whether and how to share threads.

@jsantell
Copy link
Contributor Author

jsantell commented Oct 18, 2023

Updated the implementation to now use a connection pool (r2d2), which allows us to grab a Send + !Sync DB connection, and not block on a single connection (default pool size is 10). Additionally, some performance improvements from using WAL. Implementation is much cleaner, too.

  • Connection pool implementation has 2-3x faster reads than initial worker-thread implementation.
  • WAL mode has 3x faster writes than initial worker-thread implementation, and increases read speed even more.
  • Even with these changes, reads 10^1 slower and writes 10^2 slower than rocks/sled implementations.
  • Removed stringification of cids/utf8 -- not needed
  • Use cached queries
  • Ensure any user input is sanitized/checked
  • Queries themselves could probably be improved
  • Swapped the rocksdb CI test for sqlite temporarily

@@ -98,7 +98,7 @@ jobs:
- name: 'Run Rust native target tests'
run: NOOSPHERE_LOG=deafening cargo test --features test-kubo,headers

run-test-suite-linux-rocksdb:
run-test-suite-linux-sqlite:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Temporary

@jsantell jsantell removed the request for review from cdata January 17, 2024 00:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants