Bug 1543836 - Add writeMany to kvstore r=myk
authorNan Jiang <njiang028@gmail.com>
Mon, 22 Apr 2019 18:36:38 +0000
changeset 470386 693db1b9922562f45461bb53b80a6cf34cf130d6
parent 470385 5f0139ba25448132c646dfc706866998419af77f
child 470387 55c286b28b78c3f6dc16f46550af9d05a389c927
push id35905
push userdvarga@mozilla.com
push dateTue, 23 Apr 2019 09:53:27 +0000
treeherdermozilla-central@831918f009f6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmyk
bugs1543836
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1543836 - Add writeMany to kvstore r=myk This new API allows consumer to both put and delete in batch. Differential Revision: https://phabricator.services.mozilla.com/D28153
toolkit/components/kvstore/kvstore.jsm
toolkit/components/kvstore/nsIKeyValue.idl
toolkit/components/kvstore/src/lib.rs
toolkit/components/kvstore/src/task.rs
toolkit/components/kvstore/test/xpcshell/test_kvstore.js
--- a/toolkit/components/kvstore/kvstore.jsm
+++ b/toolkit/components/kvstore/kvstore.jsm
@@ -55,78 +55,85 @@ class KeyValueService {
  * ```
  *     await database.put("foo", 1);
  *     await database.get("foo") === 1; // true
  *     await database.has("foo"); // true
  *     await database.delete("foo");
  *     await database.has("foo"); // false
  * ```
  *
- * You can also call putMany() to put multiple key/value pairs:
+ * You can also call writeMany() to put/delete multiple key/value pairs:
  *
  * ```
- *     await database.putMany({
+ *     await database.writeMany({
  *       key1: "value1",
  *       key2: "value2",
  *       key3: "value3",
+ *       key4: null, // delete
  *     });
  * ```
  *
  * And you can call its enumerate() method to retrieve a KeyValueEnumerator,
  * which is described below.
  */
 class KeyValueDatabase {
   constructor(database) {
     this.database = database;
   }
 
   put(key, value) {
     return promisify(this.database.put, key, value);
   }
 
   /**
-   * Puts multiple key/value pairs to the database.
+   * Writes multiple key/value pairs to the database.
+   *
+   * Note:
+   *   * Each write could be either put or delete.
+   *   * Given multiple values with the same key, only the last value will be stored.
+   *   * If the same key gets put and deleted for multiple times, the final state
+   *     of that key is subject to the ordering of the put(s) and delete(s).
    *
    * @param pairs Pairs could be any of following types:
    *        * An Object, all its properties and the corresponding values will
-   *          be used as key value pairs.
-   *        * A Map.
-   *        * An Array or an iterable whose elements are key-values pairs, such
-   *          as [["key1", "value1"], ["key2", "value2"]]. Note: given multiple
-   *          values with the same key, only the last value will be stored.
-   *
+   *          be used as key value pairs. A property with null or undefined indicating
+   *          a deletion.
+   *        * An Array or an iterable whose elements are key-value pairs. such as
+   *          [["key1", "value1"], ["key2", "value2"]]. Use a pair with value null
+   *          to delete a key-value pair, e.g. ["delete-key", null].
+   *        * A Map. A key with null or undefined value indicating a deletion.
    * @return A promise that is fulfilled when all the key/value pairs are written
    *         to the database.
    */
-  putMany(pairs) {
+  writeMany(pairs) {
     if (!pairs) {
-      throw new Error("putMany(): unexpected argument.");
+      throw new Error("writeMany(): unexpected argument.");
     }
 
     let entries;
 
     if (pairs instanceof Map || pairs instanceof Array ||
         typeof(pairs[Symbol.iterator]) === "function") {
       try {
-        // Let Map constructor validate the argument. Although Map accepts a
-        // different set of key/value types than that of kvstore, we do not
-        // need to check that here since it will be done later.
+        // Let Map constructor validate the argument. Note that Map remembers
+        // the original insertion order of the keys, which satisfies the ordering
+        // premise of this function.
         const map = pairs instanceof Map ? pairs : new Map(pairs);
         entries = Array.from(map, ([key, value]) => ({key, value}));
       } catch (error) {
-        throw new Error("putMany(): unexpected argument.");
+        throw new Error("writeMany(): unexpected argument.");
       }
     } else if (typeof(pairs) === "object") {
       entries = Array.from(Object.entries(pairs), ([key, value]) => ({key, value}));
     } else {
-      throw new Error("putMany(): unexpected argument.");
+      throw new Error("writeMany(): unexpected argument.");
     }
 
     if (entries.length) {
-      return promisify(this.database.putMany, entries);
+      return promisify(this.database.writeMany, entries);
     }
     return Promise.resolve();
   }
 
   has(key) {
     return promisify(this.database.has, key);
   }
 
--- a/toolkit/components/kvstore/nsIKeyValue.idl
+++ b/toolkit/components/kvstore/nsIKeyValue.idl
@@ -63,26 +63,36 @@ interface nsIKeyValueDatabase : nsISuppo
     void put(
         in nsIKeyValueVoidCallback callback,
         in AUTF8String key,
         in nsIVariant value);
 
     /**
      * Write multiple key/value pairs to the database.
      *
+     * It supports two types of write:
+     *   * Put a key/value pair into the database. It takes a nsIKeyValuePair
+     *     where its key and value follow the same types as the put() method.
+     *   * Delete a key/value pair from database. It takes a nsIkeyValuePair
+     *     where its value property must be null or undefined.
+     *
      * This features the "all-or-nothing" semantics, i.e. if any error occurs
-     * during the call, it will rollback the previous puts and terminate the
-     * put. In addition, putMany should be more efficient than calling "put"
-     * for every single key/value pair since it does all the puts in a single
-     * transaction.
+     * during the call, it will rollback the previous writes and terminate the
+     * call. In addition, writeMany should be more efficient than calling "put"
+     * or "delete" for every single key/value pair since it does all the writes
+     * in a single transaction.
      *
-     * Note: if there are multiple values with the same key in the specified
-     * pairs, only the last value will be stored in the database.
+     * Note:
+     *   * If there are multiple values with the same key in the specified
+     *     pairs, only the last value will be stored in the database.
+     *   * Deleting a key that is not in the database will be silently ignored.
+     *   * If the same key gets put and deleted for multiple times, the final
+     *     state of that key is subject to the ordering of the put(s) and delete(s).
      */
-    void putMany(
+    void writeMany(
         in nsIKeyValueVoidCallback callback,
         in Array<nsIKeyValuePair> pairs);
 
     /**
      * Retrieve the value of the specified key from the database.
      *
      * If the key/value pair doesn't exist in the database, and you specify
      * a default value, then the default value will be returned.  Otherwise,
--- a/toolkit/components/kvstore/src/lib.rs
+++ b/toolkit/components/kvstore/src/lib.rs
@@ -34,17 +34,17 @@ use owned_value::{owned_to_variant, vari
 use rkv::{OwnedValue, Rkv, SingleStore};
 use std::{
     ptr,
     sync::{Arc, RwLock},
     vec::IntoIter,
 };
 use task::{
     ClearTask, DeleteTask, EnumerateTask, GetOrCreateTask, GetTask, HasTask,
-    PutTask, PutManyTask,
+    PutTask, WriteManyTask,
 };
 use thin_vec::ThinVec;
 use xpcom::{
     interfaces::{
         nsIKeyValueDatabaseCallback, nsIKeyValueEnumeratorCallback, nsIKeyValuePair,
         nsIKeyValueVariantCallback, nsIKeyValueVoidCallback, nsISupports, nsIThread, nsIVariant,
     },
     getter_addrefs, nsIID, RefPtr, ThreadBoundRefPtr, xpcom, xpcom_method,
@@ -181,23 +181,23 @@ impl KeyValueDatabase {
         ));
 
         let thread = self.thread.get_ref().ok_or(NS_ERROR_FAILURE)?;
 
         TaskRunnable::new("KVDatabase::Put", task)?.dispatch(thread)
     }
 
     xpcom_method!(
-        put_many => PutMany(
+        write_many => WriteMany(
             callback: *const nsIKeyValueVoidCallback,
             pairs: *const ThinVec<RefPtr<nsIKeyValuePair>>
         )
     );
 
-    fn put_many(
+    fn write_many(
         &self,
         callback: &nsIKeyValueVoidCallback,
         pairs: &ThinVec<RefPtr<nsIKeyValuePair>>
     ) -> Result<(), nsresult> {
         let mut entries = Vec::with_capacity(pairs.len());
 
         for pair in pairs {
             let mut key = nsCString::new();
@@ -205,31 +205,30 @@ impl KeyValueDatabase {
                 pair.GetKey(&mut *key)
             }.to_result()?;
             if key.is_empty() {
                 return Err(nsresult::from(KeyValueError::UnexpectedValue));
             }
 
             let val: RefPtr<nsIVariant> =
                 getter_addrefs(|p| unsafe { pair.GetValue(p) })?;
-            let value = variant_to_owned(&val)?
-                .ok_or(KeyValueError::UnexpectedValue)?;
+            let value = variant_to_owned(&val)?;
             entries.push((key, value));
         }
 
-        let task = Box::new(PutManyTask::new(
+        let task = Box::new(WriteManyTask::new(
             RefPtr::new(callback),
             Arc::clone(&self.rkv),
             self.store,
             entries,
         ));
 
         let thread = self.thread.get_ref().ok_or(NS_ERROR_FAILURE)?;
 
-        TaskRunnable::new("KVDatabase::PutMany", task)?.dispatch(thread)
+        TaskRunnable::new("KVDatabase::WriteMany", task)?.dispatch(thread)
     }
 
     xpcom_method!(
         get => Get(
             callback: *const nsIKeyValueVariantCallback,
             key: *const nsACString,
             default_value: *const nsIVariant
         )
--- a/toolkit/components/kvstore/src/task.rs
+++ b/toolkit/components/kvstore/src/task.rs
@@ -177,52 +177,66 @@ impl Task for PutTask {
 
             Ok(())
         }()));
     }
 
     task_done!(void);
 }
 
-pub struct PutManyTask {
+pub struct WriteManyTask {
     callback: AtomicCell<Option<ThreadBoundRefPtr<nsIKeyValueVoidCallback>>>,
     rkv: Arc<RwLock<Rkv>>,
     store: SingleStore,
-    pairs: Vec<(nsCString, OwnedValue)>,
+    pairs: Vec<(nsCString, Option<OwnedValue>)>,
     result: AtomicCell<Option<Result<(), KeyValueError>>>,
 }
 
-impl PutManyTask {
+impl WriteManyTask {
     pub fn new(
         callback: RefPtr<nsIKeyValueVoidCallback>,
         rkv: Arc<RwLock<Rkv>>,
         store: SingleStore,
-        pairs: Vec<(nsCString, OwnedValue)>,
-    ) -> PutManyTask {
-        PutManyTask {
+        pairs: Vec<(nsCString, Option<OwnedValue>)>,
+    ) -> WriteManyTask {
+        WriteManyTask {
             callback: AtomicCell::new(Some(ThreadBoundRefPtr::new(callback))),
             rkv,
             store,
             pairs,
             result: AtomicCell::default(),
         }
     }
 }
 
-impl Task for PutManyTask {
+impl Task for WriteManyTask {
     fn run(&self) {
         // We do the work within a closure that returns a Result so we can
         // use the ? operator to simplify the implementation.
         self.result.store(Some(|| -> Result<(), KeyValueError> {
             let env = self.rkv.read()?;
             let mut writer = env.write()?;
 
             for (key, value) in self.pairs.iter() {
                 let key = str::from_utf8(key)?;
-                self.store.put(&mut writer, key, &Value::from(value))?;
+                match value {
+                    Some(val) => self.store.put(&mut writer, key, &Value::from(val))?,
+                    None => {
+                        match self.store.delete(&mut writer, key) {
+                            Ok(_) => (),
+
+                            // LMDB fails with an error if the key to delete wasn't found,
+                            // and Rkv returns that error, but we ignore it, as we expect most
+                            // of our consumers to want this behavior.
+                            Err(StoreError::LmdbError(lmdb::Error::NotFound)) => (),
+
+                            Err(err) => return Err(KeyValueError::StoreError(err)),
+                        };
+                    }
+                }
             }
             writer.commit()?;
 
             Ok(())
         }()));
     }
 
     task_done!(void);
--- a/toolkit/components/kvstore/test/xpcshell/test_kvstore.js
+++ b/toolkit/components/kvstore/test/xpcshell/test_kvstore.js
@@ -150,75 +150,164 @@ add_task(async function clear() {
 
   Assert.strictEqual(await database.clear(), undefined);
   Assert.strictEqual(await database.has("int-key"), false);
   Assert.strictEqual(await database.has("double-key"), false);
   Assert.strictEqual(await database.has("string-key"), false);
   Assert.strictEqual(await database.has("bool-key"), false);
 });
 
-add_task(async function putMany() {
-  const databaseDir = await makeDatabaseDir("putMany");
+add_task(async function writeManyFailureCases() {
+  const databaseDir = await makeDatabaseDir("writeManyFailureCases");
+  const database = await KeyValueService.getOrCreate(databaseDir, "db");
+
+  Assert.throws(() => database.writeMany(), /unexpected argument/);
+  Assert.throws(() => database.writeMany("foo"), /unexpected argument/);
+  Assert.throws(() => database.writeMany(["foo"]), /unexpected argument/);
+});
+
+add_task(async function writeManyPutOnly() {
+  const databaseDir = await makeDatabaseDir("writeMany");
   const database = await KeyValueService.getOrCreate(databaseDir, "db");
 
   async function test_helper(pairs) {
-    Assert.strictEqual(await database.putMany(pairs), undefined);
+    Assert.strictEqual(await database.writeMany(pairs), undefined);
     Assert.strictEqual(await database.get("int-key"), 1234);
     Assert.strictEqual(await database.get("double-key"), 56.78);
     Assert.strictEqual(await database.get("string-key"), "Héllo, wőrld!");
     Assert.strictEqual(await database.get("bool-key"), true);
     await database.clear();
   }
 
-  // putMany with an empty object is OK
-  Assert.strictEqual(await database.putMany({}), undefined);
+  // writeMany with an empty object is OK
+  Assert.strictEqual(await database.writeMany({}), undefined);
 
-  // putMany with an object
+  // writeMany with an object
   const pairs = {
     "int-key": 1234,
     "double-key": 56.78,
     "string-key": "Héllo, wőrld!",
     "bool-key": true,
   };
   await test_helper(pairs);
 
-  // putMany with an array of pairs
+  // writeMany with an array of pairs
   const arrayPairs = [
     ["int-key", 1234],
     ["double-key", 56.78],
     ["string-key", "Héllo, wőrld!"],
     ["bool-key", true],
   ];
   await test_helper(arrayPairs);
 
-  // putMany with a key/value generator
+  // writeMany with a key/value generator
   function* pairMaker() {
     yield ["int-key", 1234];
     yield ["double-key", 56.78];
     yield ["string-key", "Héllo, wőrld!"];
     yield ["bool-key", true];
   }
   await test_helper(pairMaker());
 
-  // putMany with a map
+  // writeMany with a map
   const mapPairs = new Map(arrayPairs);
   await test_helper(mapPairs);
 });
 
-add_task(async function putManyFailureCases() {
-  const databaseDir = await makeDatabaseDir("putManyFailureCases");
+add_task(async function writeManyDeleteOnly() {
+  const databaseDir = await makeDatabaseDir("writeManyDeletesOnly");
   const database = await KeyValueService.getOrCreate(databaseDir, "db");
 
-  Assert.throws(() => database.putMany(), /unexpected argument/);
-  Assert.throws(() => database.putMany("foo"), /unexpected argument/);
-  Assert.throws(() => database.putMany(["foo"]), /unexpected argument/);
+  // writeMany with an object
+  const pairs = {
+    "int-key": 1234,
+    "double-key": 56.78,
+    "string-key": "Héllo, wőrld!",
+    "bool-key": true,
+  };
+
+  async function test_helper(deletes) {
+    Assert.strictEqual(await database.writeMany(pairs), undefined);
+    Assert.strictEqual(await database.writeMany(deletes), undefined);
+    Assert.strictEqual(await database.get("int-key"), null);
+    Assert.strictEqual(await database.get("double-key"), null);
+    Assert.strictEqual(await database.get("string-key"), null);
+    Assert.strictEqual(await database.get("bool-key"), null);
+  }
+
+  // writeMany with an empty object is OK
+  Assert.strictEqual(await database.writeMany({}), undefined);
+
+  // writeMany with an object
+  await test_helper({
+    "int-key": null,
+    "double-key": null,
+    "string-key": null,
+    "bool-key": null,
+  });
+
+  // writeMany with an array of pairs
+  const arrayPairs = [
+    ["int-key", null],
+    ["double-key", null],
+    ["string-key", null],
+    ["bool-key", null],
+  ];
+  await test_helper(arrayPairs);
 
-  const pairWithoutValue = {"key": undefined};
-  await Assert.rejects(database.putMany(pairWithoutValue), /NS_ERROR_UNEXPECTED/);
-  await Assert.rejects(database.putMany([["foo"]]), /NS_ERROR_UNEXPECTED/);
+  // writeMany with a key/value generator
+  function* pairMaker() {
+    yield ["int-key", null];
+    yield ["double-key", null];
+    yield ["string-key", null];
+    yield ["bool-key", null];
+  }
+  await test_helper(pairMaker());
+
+  // writeMany with a map
+  const mapPairs = new Map(arrayPairs);
+  await test_helper(mapPairs);
+});
+
+add_task(async function writeManyPutDelete() {
+  const databaseDir = await makeDatabaseDir("writeManyPutDelete");
+  const database = await KeyValueService.getOrCreate(databaseDir, "db");
+
+  await database.writeMany([
+    ["key1", "val1"],
+    ["key3", "val3"],
+    ["key4", "val4"],
+    ["key5", "val5"],
+  ]);
+
+  await database.writeMany([
+    ["key2", "val2"],
+    ["key4", null],
+    ["key5", null],
+  ]);
+
+  Assert.strictEqual(await database.get("key1"), "val1");
+  Assert.strictEqual(await database.get("key2"), "val2");
+  Assert.strictEqual(await database.get("key3"), "val3");
+  Assert.strictEqual(await database.get("key4"), null);
+  Assert.strictEqual(await database.get("key5"), null);
+
+  await database.clear();
+
+  await database.writeMany([
+    ["key1", "val1"],
+    ["key1", null],
+    ["key1", "val11"],
+    ["key1", null],
+    ["key2", null],
+    ["key2", "val2"],
+  ]);
+
+  Assert.strictEqual(await database.get("key1"), null);
+  Assert.strictEqual(await database.get("key2"), "val2");
 });
 
 add_task(async function getOrCreateNamedDatabases() {
   const databaseDir = await makeDatabaseDir("getOrCreateNamedDatabases");
 
   let fooDB = await KeyValueService.getOrCreate(databaseDir, "foo");
   Assert.ok(fooDB, "retrieval of first named database works");