Use Rc::try_unwrap for (possibly?) better performance (#85)

* Benchmarks first

* Use `Rc::try_unwrap` in `Node::set_value`

And in `Node::remove_value`

* Remove `RemoveResult` type alias

* Add invariant checks on deserialization

* Merge a level of `if`s into `match`

* clippy: Use non-panicing `Utc.timestamp_opt`

wnfs-bench/hamt.rs

@@ -2,7 +2,7 @@ use criterion::{
     async_executor::AsyncStdExecutor, black_box, criterion_group, criterion_main, BatchSize,
     Criterion, Throughput,
 };
-use proptest::{arbitrary::any, test_runner::TestRunner};
+use proptest::{arbitrary::any, collection::vec, test_runner::TestRunner};
 use std::{rc::Rc, sync::Arc};
 use wnfs::{
     dagcbor,
@@ -19,7 +19,7 @@ fn node_set(c: &mut Criterion) {
     let mut store = MemoryBlockStore::default();
     let operations = operations(any::<[u8; 32]>(), any::<u64>(), 1_000_000).sample(&mut runner);
     let node =
-        &*async_std::task::block_on(async { node_from_operations(operations, &mut store).await })
+        &async_std::task::block_on(async { node_from_operations(operations, &mut store).await })
             .expect("Couldn't setup HAMT node from operations");
 
     let store = Arc::new(store);
@@ -32,7 +32,39 @@ fn node_set(c: &mut Criterion) {
                 (store, kv)
             },
             |(store, (key, value))| async move {
-                black_box(node.set(key, value, store.as_ref()).await.unwrap());
+                black_box(
+                    Rc::clone(node)
+                        .set(key, value, store.as_ref())
+                        .await
+                        .unwrap(),
+                );
+            },
+            BatchSize::SmallInput,
+        );
+    });
+}
+
+fn node_set_consecutive(c: &mut Criterion) {
+    let mut runner = TestRunner::deterministic();
+
+    c.bench_function("node set 1000 consecutive", |b| {
+        b.to_async(AsyncStdExecutor).iter_batched(
+            || {
+                let mut store = MemoryBlockStore::default();
+                let operations =
+                    operations(any::<[u8; 32]>(), any::<u64>(), 1000).sample(&mut runner);
+                let node = async_std::task::block_on(async {
+                    node_from_operations(operations, &mut store).await
+                })
+                .expect("Couldn't setup HAMT node from operations");
+
+                let kvs = vec((any::<[u8; 32]>(), any::<u64>()), 1000).sample(&mut runner);
+                (node, store, kvs)
+            },
+            |(mut node, store, kvs)| async move {
+                for (key, value) in kvs {
+                    node = black_box(node.set(key, value, &store).await.unwrap());
+                }
             },
             BatchSize::SmallInput,
         );
@@ -159,6 +191,7 @@ fn hamt_set_encode(c: &mut Criterion) {
 criterion_group!(
     benches,
     node_set,
+    node_set_consecutive,
     node_load_get,
     node_load_remove,
     hamt_load_decode,

wnfs/src/common/metadata.rs

@@ -94,9 +94,12 @@ impl Metadata {
     /// let imprecise_time = Utc.timestamp(time.timestamp(), 0);
     /// assert_eq!(metadata.get_created(), Some(imprecise_time));
     /// ```
+    ///
+    /// Will return `None` if there's no created metadata on the
+    /// node or if it's not a second-based POSIX timestamp integer.
     pub fn get_created(&self) -> Option<DateTime<Utc>> {
         self.0.get("created").and_then(|ipld| match ipld {
-            Ipld::Integer(i) => Some(Utc.timestamp(i64::try_from(*i).ok()?, 0)),
+            Ipld::Integer(i) => Utc.timestamp_opt(i64::try_from(*i).ok()?, 0).single(),
             _ => None,
         })
     }
@@ -115,9 +118,12 @@ impl Metadata {
     /// let imprecise_time = Utc.timestamp(time.timestamp(), 0);
     /// assert_eq!(metadata.get_modified(), Some(imprecise_time));
     /// ```
+    ///
+    /// Will return `None` if there's no created metadata on the
+    /// node or if it's not a second-based POSIX timestamp integer.
     pub fn get_modified(&self) -> Option<DateTime<Utc>> {
         self.0.get("modified").and_then(|ipld| match ipld {
-            Ipld::Integer(i) => Some(Utc.timestamp(i64::try_from(*i).ok()?, 0)),
+            Ipld::Integer(i) => Utc.timestamp_opt(i64::try_from(*i).ok()?, 0).single(),
             _ => None,
         })
     }

wnfs/src/private/forest.rs

@@ -237,7 +237,6 @@ impl PrivateForest {
         value: Cid,
         store: &mut B,
     ) -> Result<Rc<Self>> {
-        let mut cloned = (*self).clone();
         // TODO(matheus23): This iterates the path in the HAMT twice.
         // We could consider implementing something like upsert instead.
         let mut values = self
@@ -247,8 +246,10 @@ impl PrivateForest {
             .cloned()
             .unwrap_or_default();
         values.insert(value);
-        cloned.root = self.root.set(name, values, store).await?;
-        Ok(Rc::new(cloned))
+
+        let mut hamt = Rc::try_unwrap(self).unwrap_or_else(|rc| (*rc).clone());
+        hamt.root = hamt.root.set(name, values, store).await?;
+        Ok(Rc::new(hamt))
     }
 
     /// Gets the encrypted value at the given key.