feat: Introduce "--storage-memory-cache-limit" #671

jsantell · 2023-10-06T23:18:36Z

Introduce ConfigurableStorage trait to open storage with generalized configurations (only memory_cache_limit at the moment)
Introduce --storage-memory-cache-limit and NoosphereStorage { config: NoosphereStorageConfig, path: NoosphereStoragePath } for propagating these values from orb and rust API respectively into the sphere builder, which makes its way into the storage provider.
Make invoke_cli() take a mutable Workspace (for setting storage config)

rust/noosphere/tests/cli.rs

rust/noosphere/src/storage.rs

jsantell · 2023-10-09T20:01:26Z

Sled: Limits the system page cache capacity.
RocksDB: Memory usage in RocksDB can be attributed to the block cache, indexes/filter blocks, and memtable. This config field limits the overall memtable cache usage across all column families (stores). The block cache is 32MB by default. May need to tweak these settings more in the future.
IndexedDb: Does not use this value. I imagine IndexedDb will eventually have some storage configurations where it'd make sense to implement ConfigurationStorage for it, ignoring the unsupported values.

Other ideas:

A supports_config() method in the trait if we need to concretely know how supported a config is (enum ConfigSupport { Unsupported, PartialSupport, Supported })
Dumping internal stats from a Storage (RocksDB has a lot of insight here) via maybe a timer or gateway API

cdata · 2023-10-10T20:13:25Z

rust/noosphere-cli/src/native/cli.rs

+
+        /// If set, the amount of memory that the storage provider may use
+        /// for caching in bytes.
+        #[clap(long)]


Can we get a short arg for this? e.g., short = 'S' or something

Punting on this for now, as this is a low-level operator command

rust/noosphere-cli/src/native/commands/serve.rs

rust/noosphere-cli/src/native/commands/sphere/mod.rs

rust/noosphere-cli/src/native/helpers/cli.rs

rust/noosphere-cli/src/native/workspace.rs

cdata · 2023-10-10T20:22:28Z

rust/noosphere-storage/src/implementation/sled.rs

@@ -54,6 +58,25 @@ impl Storage for SledStorage {
    }
 }

+#[async_trait]
+impl ConfigurableStorage for SledStorage {


It seems like this implementation is the same for both SledStorage and RocksDbStorage. Can it just be a general default implementation?

I don't think so, they both just coincidentally have the core of their instantiation and setup in Self::with_config rather than calling <Self as ConfigurableStorage>::open_with_config from Self::new

cdata · 2023-10-10T20:25:46Z

rust/noosphere/src/storage.rs

+        IpfsStorage::new(storage, maybe_client)
+    };
+
+    debug!("Creating platform storage: {:#?}", storage);


This log line might be better written before the storage is created (otherwise change to past tense e.g., "created" not "creating"; but, I think moving it is better so that you get appropriate context for any subsequent error).

Changed to created, as we use storage's Debug implementation to log storage type and extra configurations (could add more debugging to type if needed e.g. trait StorageInfo { fn name() -> String }

cdata

Great work, thanks for adding this capability.

jsantell · 2023-10-11T19:44:06Z

rust/noosphere-cli/src/native/mod.rs

+/// creates a new [Workspace] from the current working directory.
+///
+/// Use [invoke_cli_with_workspace] if using your own [Workspace].
+pub async fn invoke_cli<'a>(cli: Cli, context: &CliContext<'a>) -> Result<()> {


invoke_cli now creates a Workspace from its args rather than accept one so that we can always have the correct storage configuration on instantiation (this is the bulk of the follow up changes)

to limit size of memory cache in storage providers.

cdata

Looks great, thanks for implementing this!

jsantell commented Oct 6, 2023

View reviewed changes

rust/noosphere/tests/cli.rs Outdated Show resolved Hide resolved

rust/noosphere/src/storage.rs Show resolved Hide resolved

jsantell force-pushed the storage-memory-limit branch 6 times, most recently from 67f2ff5 to 1544059 Compare October 9, 2023 18:05

jsantell marked this pull request as ready for review October 9, 2023 18:11

jsantell requested a review from cdata October 9, 2023 18:27