Issue 6856 - Remove FilterStorage.moveSubscription

lib/filterStorage.js

Index: lib/filterStorage.js
===================================================================
--- a/lib/filterStorage.js
+++ b/lib/filterStorage.js
@@ -75,37 +75,49 @@
 
   /**
    * Map of properties listed in the filter storage file before the sections
    * start. Right now this should be only the format version.
    */
   fileProperties: Object.create(null),
 
   /**
-   * List of filter subscriptions containing all filters
-   * @type {Subscription[]}
+   * Yields subscriptions containing all filters
+   * @yields {Subscription}
    */
-  subscriptions: [],
+  *subscriptions()
+  {
+    yield* this.knownSubscriptions.values();
+  },
+
+  /**
+   * Number of known subscriptions.
+   * @returns {number}

[Manish Jethani, 2018/09/27 17:42:54]

This should be `@type {number}` since it's a property, not a method.

[Jon Sonesen, 2018/10/01 03:02:22]

Done.

+   */
+  get subscriptionCount()
+  {
+    return this.knownSubscriptions.size;
+  },
 
   /**
    * Map of subscriptions already on the list, by their URL/identifier
    * @type {Map.<string,Subscription>}
    */
   knownSubscriptions: new Map(),
 
   /**
    * Finds the filter group that a filter should be added to by default. Will
    * return null if this group doesn't exist yet.
    * @param {Filter} filter
    * @return {?SpecialSubscription}
    */
   getGroupForFilter(filter)
   {
     let generalSubscription = null;
-    for (let subscription of FilterStorage.subscriptions)
+    for (let subscription of FilterStorage.subscriptions())
     {
       if (subscription instanceof SpecialSubscription && !subscription.disabled)
       {
         // Always prefer specialized subscriptions
         if (subscription.isDefaultFor(filter))
           return subscription;
 
         // If this is a general subscription - store it as fallback
@@ -123,75 +135,40 @@
    * Adds a filter subscription to the list
    * @param {Subscription} subscription filter subscription to be added
    */
   addSubscription(subscription)
   {
     if (FilterStorage.knownSubscriptions.has(subscription.url))
       return;
 
-    FilterStorage.subscriptions.push(subscription);
     FilterStorage.knownSubscriptions.set(subscription.url, subscription);
     addSubscriptionFilters(subscription);
 
     filterNotifier.emit("subscription.added", subscription);
   },
 
   /**
    * Removes a filter subscription from the list
    * @param {Subscription} subscription filter subscription to be removed
    */
   removeSubscription(subscription)
   {
-    for (let i = 0; i < FilterStorage.subscriptions.length; i++)
-    {
-      if (FilterStorage.subscriptions[i].url == subscription.url)
-      {
-        removeSubscriptionFilters(subscription);
-
-        FilterStorage.subscriptions.splice(i--, 1);
-        FilterStorage.knownSubscriptions.delete(subscription.url);
-
-        // This should be the last remaining reference to the Subscription
-        // object.
-        Subscription.knownSubscriptions.delete(subscription.url);
-
-        filterNotifier.emit("subscription.removed", subscription);
-        return;
-      }
-    }
-  },
-
-  /**
-   * Moves a subscription in the list to a new position.
-   * @param {Subscription} subscription filter subscription to be moved
-   * @param {Subscription} [insertBefore] filter subscription to insert before
-   *        (if omitted the subscription will be put at the end of the list)
-   */
-  moveSubscription(subscription, insertBefore)
-  {
-    let currentPos = FilterStorage.subscriptions.indexOf(subscription);
-    if (currentPos < 0)
+    if (!FilterStorage.knownSubscriptions.has(subscription.url))
       return;
 
-    let newPos = -1;
-    if (insertBefore)
-      newPos = FilterStorage.subscriptions.indexOf(insertBefore);
+    removeSubscriptionFilters(subscription);
 
-    if (newPos < 0)
-      newPos = FilterStorage.subscriptions.length;
+    FilterStorage.knownSubscriptions.delete(subscription.url);
 
-    if (currentPos < newPos)
-      newPos--;
-    if (currentPos == newPos)
-      return;
+    // This should be the last remaining reference to the Subscription
+    // object.
+    Subscription.knownSubscriptions.delete(subscription.url);
 
-    FilterStorage.subscriptions.splice(currentPos, 1);
-    FilterStorage.subscriptions.splice(newPos, 0, subscription);
-    filterNotifier.emit("subscription.moved", subscription);
+    filterNotifier.emit("subscription.removed", subscription);
   },
 
   /**
    * Replaces the list of filters in a subscription by a new list
    * @param {Subscription} subscription filter subscription to be updated
    * @param {Filter[]} filters new filter list
    */
   updateSubscriptionFilters(subscription, filters)
@@ -368,17 +345,16 @@
       parser.process(line);
       if (line === null)
       {
         let knownSubscriptions = new Map();
         for (let subscription of parser.subscriptions)
           knownSubscriptions.set(subscription.url, subscription);
 
         this.fileProperties = parser.fileProperties;
-        this.subscriptions = parser.subscriptions;
         this.knownSubscriptions = knownSubscriptions;
         Filter.knownFilters = parser.knownFilters;
         Subscription.knownSubscriptions = parser.knownSubscriptions;
 
         if (!silent)
           filterNotifier.emit("load");
       }
     };
@@ -389,17 +365,17 @@
    * @return {Promise} promise resolved or rejected when loading is complete
    */
   loadFromDisk()
   {
     let tryBackup = backupIndex =>
     {
       return this.restoreBackup(backupIndex, true).then(() =>
       {
-        if (this.subscriptions.length == 0)
+        if (this.subscriptionCount == 0)

[Manish Jethani, 2018/10/02 00:41:29]

You mean to change this to `this.knownSubscriptions.size` as well?

           return tryBackup(backupIndex + 1);
       }).catch(error =>
       {
         // Give up
       });
     };
 
     return IO.statFile(this.sourceFile).then(statData =>
@@ -409,17 +385,17 @@
         this.firstRun = true;
         return;
       }
 
       let parser = this.importData(true);
       return IO.readFromFile(this.sourceFile, parser).then(() =>
       {
         parser(null);
-        if (this.subscriptions.length == 0)
+        if (this.subscriptionCount == 0)
         {
           // No filter subscriptions in the file, this isn't right.
           throw new Error("No data in the file");
         }
       });
     }).catch(error =>
     {
       Cu.reportError(error);
@@ -463,17 +439,17 @@
   },
 
   /**
    * Generator serializing filter data and yielding it line by line.
    */
   *exportData()
   {
     // Do not persist external subscriptions
-    let subscriptions = this.subscriptions.filter(
+    let subscriptions = [...this.subscriptions()].filter(

[Manish Jethani, 2018/09/27 17:42:54]

I think we should remove this and just add the following to the loops below:

  if (subscription instanceof ExternalSubscription)
    continue;

[Jon Sonesen, 2018/09/27 18:46:59]

Can the iterations be consolidated as well?

[Manish Jethani, 2018/09/28 08:15:49]

It doesn't look like, because all the subscriptions need to be serialized first
before each subscription's filters.

We would already be avoiding two loops of sorts by skipping that code, but then
we'd have to do an additional `instanceof` check for each subscription.

How about this instead:

  let exportableSubscriptions = [];
  for (let subscription of this._subscriptions())
  {
    if (!(subscription instanceof ExternalSubscription))
    exportableSubscriptions.push(subscription);
  }

So basically I'm saying we inline the call to the filter method and skip the
creation of an additional temporary array.

[Manish Jethani, 2018/09/28 22:33:25]

OK, scratch this. Let's leave it as it is (in the current patch).

The reason I'm saying is that I now realize that global state, including the
contents of the knownSubscriptions map, can change during the execution of this
method. Remember this is a generator, which means it exits at each yield
statement, then comes back to where it left (possibly after a million years).
This means, technically, we should make a copy of the _current_ subscriptions to
export.

I would say push to a temporary array using a `for` loop, but it seems the
spread operator is very efficient.

https://jsperf.com/set-iterator-vs-foreach/4

Anyway, this is a hotspot, so we can revisit this function when we're
optimizing. This looks OK to me for now.

[Jon Sonesen, 2018/10/01 03:02:23]

Acknowledged.

[Manish Jethani, 2018/10/02 00:41:30]

I'm sorry about this once again, but I've done an actual test now (since I've
been obsessing about related things lately). It seems using the `filter` method
here is a really bad idea. We should just push to an array instead.

  let subscriptions = [];
  for (let subscription of this.subscriptions())
  {
    if (!(subscription instanceof ExternalSubscription))
      subscriptions.push(subscription);
  }

This seems to give the best results. You can try it out, either by adding some
before/after code within the function itself or just using the JS profiler.
Otherwise you could just take my word for it. :)

       s => !(s instanceof ExternalSubscription)
     );
 
     yield "# Adblock Plus preferences";
     yield "version=" + formatVersion;
 
     let saved = new Set();
     let buf = [];