Don't update last_used if the new value is the same

This reduces time by -67% in benchmarks for continuous reads.

Author: anacrolix

src/c_api/ext_fns/handle.rs

@@ -133,6 +133,14 @@ pub extern "C" fn possum_single_read_at(
         Err(err) => return err.into(),
         Ok(ok) => ok,
     };
+    // eprintln!(
+    //     "reading single {} bytes at {} from {}, read {}: {}",
+    //     read_buf.len(),
+    //     offset,
+    //     value.length(),
+    //     r_nbyte,
+    //     rust_key.escape_ascii(),
+    // );
     buf.size = r_nbyte;
     NoError
 }

src/handle.rs

@@ -257,7 +257,8 @@ impl Handle {
     /// Begins a read transaction.
     pub fn read(&self) -> rusqlite::Result<Reader<OwnedTx>> {
         let reader = Reader {
-            owned_tx: self.start_deferred_transaction()?,
+            owned_tx: self
+                .start_writable_transaction_with_behaviour(TransactionBehavior::Immediate)?,
             reads: Default::default(),
         };
         Ok(reader)

src/lib.rs

@@ -480,10 +480,15 @@ impl Deref for Value {
 
 impl Value {
     fn from_row(row: &rusqlite::Row) -> rusqlite::Result<Self> {
-        let file_id: Option<FileId> = row.get(0)?;
-        let file_offset: Option<u64> = row.get(1)?;
-        let length = row.get(2)?;
-        let last_used = row.get(3)?;
+        Self::from_column_values(row.get(0)?, row.get(1)?, row.get(2)?, row.get(3)?)
+    }
+
+    fn from_column_values(
+        file_id: Option<FileId>,
+        file_offset: Option<u64>,
+        length: ValueLength,
+        last_used: Timestamp,
+    ) -> rusqlite::Result<Self> {
         let location = if length == 0 {
             assert_eq!(file_id, None);
             assert_eq!(file_offset, None);

src/tests.rs

@@ -3,6 +3,7 @@ use std::thread::*;
 use std::time::*;
 
 use anyhow::Result;
+use rusqlite::TransactionState;
 
 use self::test;
 use super::*;
@@ -150,3 +151,24 @@ fn test_torrent_storage_benchmark() -> anyhow::Result<()> {
     use testing::torrent_storage::*;
     BENCHMARK_OPTS.build()?.run()
 }
+
+/// Show that update moves a transaction to write, even if nothing is changed. This was an
+/// investigation on how to optimize touch_for_read if last_used doesn't change.
+#[test]
+fn test_sqlite_update_same_value_txn_state() -> Result<()> {
+    let mut conn = rusqlite::Connection::open_in_memory()?;
+    conn.execute_batch(
+        r"
+        create table a(b);
+        --insert into a values (1);
+        ",
+    )?;
+    let tx = conn.transaction()?;
+    assert_eq!(tx.transaction_state(None)?, TransactionState::None);
+    let changed = tx.execute("update a set b=1", [])?;
+    // No rows were changed.
+    assert_eq!(changed, 0);
+    // But now we're a write transaction anyway.
+    assert_eq!(tx.transaction_state(None)?, TransactionState::Write);
+    Ok(())
+}

src/tx.rs

@@ -185,15 +185,37 @@ impl<T> ReadTransaction for T where T: ReadOnlyTransactionAccessor {}
 
 impl<'h, H> Transaction<'h, H> {
     pub fn touch_for_read(&mut self, key: &[u8]) -> rusqlite::Result<Value> {
-        self.tx
-            .prepare_cached(&format!(
-                "update keys \
-                set last_used=cast(unixepoch('subsec')*1e3 as integer) \
-                where key=? \
-                returning {}",
+        // Avoid modifying the manifest. We had to take a write lock already to ensure our data
+        // isn't modified on us, but it still seems to be an improvement. (-67% on read times in
+        // fact).
+        let (file_id, file_offset, value_length, mut last_used, now) = self
+            .tx
+            .prepare_cached_readonly(&format!(
+                "select {}, cast(unixepoch('subsec')*1e3 as integer) \
+                from keys where key=?",
                 value_columns_sql()
             ))?
-            .query_row([key], Value::from_row)
+            .query_row([key], |row| row.try_into())?;
+        let update_last_used = last_used != now;
+        // eprintln!("updating last used: {}", update_last_used);
+        if update_last_used {
+            let (new_last_used,) = self
+                .tx
+                .prepare_cached(
+                    r"
+                    update keys
+                    set last_used=cast(unixepoch('subsec')*1e3 as integer)
+                    where key=?
+                    returning last_used
+                    ",
+                )?
+                .query_row([key], |row| row.try_into())?;
+            // This can in fact change between calls. Since we're updating now anyway, we don't
+            // really care.
+            //assert_eq!(new_last_used, now);
+            last_used = new_last_used;
+        }
+        Value::from_column_values(file_id, file_offset, value_length, last_used)
     }
 }