Issue 7096 - Make it possible to lazy initialize a filter's subscriptions

lib/filterStorage.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 110 matching lines @@
   /**
    * The number of subscriptions in the storage.
    * @type {number}
    */
   get subscriptionCount()
   {
     return this.knownSubscriptions.size;
   }
 
   /**
-   * Adds any subscriptions to which a filter belongs to the filter.
+   * Yields subscriptions in the storage to which the given filter belongs.
    * @param {Filter} filter
-   */
-  addSubscriptionsToFilter(filter)
-  {
-    for (let subscription of this.subscriptions())
-    {
-      if (subscription.filters.indexOf(filter) != -1)

[hub, 2018/11/10 04:33:21]

this is quite an expensive search, isn't it?

[Manish Jethani, 2018/11/15 22:58:52]

Yes, so here's the thing:

 1.  This function will not be executed on startup, because the initial filter
objects will already have their subscriptions assigned in INIParser; once those
objects are discarded and then when a subscription is updated, it will be called
for any newly created filter objects

 2.  In
https://codereview.adblockplus.org/29934588/diff/29934599/lib/subscriptionCla...
I intend to add a Set object containing the filter text, so this will become a
set lookup and it will be faster (see searchFilter method)

[hub, 2018/11/16 17:04:39]

So shall we make sure the review 29934588 lands first?

[Manish Jethani, 2018/11/17 19:49:17]

OK, let's do this one first instead, it's easier:

https://codereview.adblockplus.org/29935564/

I'll prepare the other one.

-        filter.addSubscription(subscription);
-    }
-
-    filter.subscriptionsAssigned = true;
+   * @yields {Subscription}
+   */
+  *subscriptionsForFilter(filter)
+  {
+    if (!filter.subscriptionsAdded)
+      addSubscriptionsToFilter(filter);
+
+    yield* filter.subscriptions();
+  }
+
+  /**
+   * Returns the number of subscriptions in the storage to which the given
+   * filter belongs.
+   * @param {Filter} filter
+   * @return {number}
+   */
+  getSubscriptionCountForFilter(filter)
+  {
+    if (!filter.subscriptionsAdded)
+      addSubscriptionsToFilter(filter);
+
+    return filter.subscriptionCount;
   }
 
   /**
    * Finds the filter group that a filter should be added to by default. Will
    * return <code>null</code> if this group doesn't exist yet.
    * @param {Filter} filter
    * @returns {?SpecialSubscription}
    */
   getGroupForFilter(filter)
   {
@@ skipping 23 matching lines @@
    */
   addSubscription(subscription)
   {
     if (this.knownSubscriptions.has(subscription.url))
       return;
 
     this.knownSubscriptions.set(subscription.url, subscription);
     connectSubscriptionFilters(subscription);
 
     filterNotifier.emit("subscription.added", subscription);
+
+    subscription.clearCaches();
   }
 
   /**
    * Removes a subscription from the storage.
    * @param {Subscription} subscription The subscription to be removed.
    */
   removeSubscription(subscription)
   {
     if (!this.knownSubscriptions.has(subscription.url))
       return;
 
     disconnectSubscriptionFilters(subscription);
 
     this.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);
   }
 
   /**
    * Replaces the list of filters in a subscription with a new list.
    * @param {Subscription} subscription The subscription to be updated.
    * @param {Array.<Filter>} filters The new list of filters.
    */
   updateSubscriptionFilters(subscription, filters)
   {
-    disconnectSubscriptionFilters(subscription);
-    let oldFilters = subscription.filters;
-    subscription.filters = filters;
-    connectSubscriptionFilters(subscription);
+    let oldFilters = [...subscription.filters()];
+    disconnectSubscriptionFilters(subscription, oldFilters);
+    subscription.clearFilters();
+
+    for (let filter of filters)
+      subscription.addFilter(filter);
+
+    connectSubscriptionFilters(subscription, filters);
+
     filterNotifier.emit("subscription.updated", subscription, oldFilters);
+
+    subscription.clearCaches();
   }
 
   /**
    * Adds a user-defined filter to the storage.
    * @param {Filter} filter
    * @param {?SpecialSubscription} [subscription] The subscription that the
    *   filter should be added to.
    * @param {number} [position] The position within the subscription at which
    *   the filter should be added. If not specified, the filter is added at the
    *   end of the subscription.
    */
   addFilter(filter, subscription, position)
   {
     if (!subscription)
     {
-      for (let currentSubscription of filter.subscriptions())
+      for (let currentSubscription of this.subscriptionsForFilter(filter))
       {
         if (currentSubscription instanceof SpecialSubscription &&
             !currentSubscription.disabled)
         {
           return;   // No need to add
         }
       }
       subscription = this.getGroupForFilter(filter);
     }
     if (!subscription)
     {
       // No group for this filter exists, create one
       subscription = SpecialSubscription.createForFilter(filter);
       this.addSubscription(subscription);
       return;
     }
 
     if (typeof position == "undefined")
-      position = subscription.filters.length;
+      position = subscription.filterCount;
 
     filter.addSubscription(subscription);
-    subscription.filters.splice(position, 0, filter);
+    subscription.insertFilterAt(filter, position);
     filterNotifier.emit("filter.added", filter, subscription, position);
   }
 
   /**
    * Removes a user-defined filter from the storage.
    * @param {Filter} filter
    * @param {?SpecialSubscription} [subscription] The subscription that the
    *   filter should be removed from. If not specified, the filter will be
    *   removed from all subscriptions.
    * @param {number} [position] The position within the subscription at which
    *   the filter should be removed. If not specified, all instances of the
    *   filter will be removed.
    */
   removeFilter(filter, subscription, position)
   {
     let subscriptions = (
-      subscription ? [subscription] : filter.subscriptions()
+      subscription ? [subscription] : this.subscriptionsForFilter(filter)
     );
     for (let currentSubscription of subscriptions)
     {
       if (currentSubscription instanceof SpecialSubscription)
       {
         let positions = [];
         if (typeof position == "undefined")
         {
           let index = -1;
           do
           {
-            index = currentSubscription.filters.indexOf(filter, index + 1);
+            index = currentSubscription.searchFilter(filter, index + 1);
             if (index >= 0)
               positions.push(index);
           } while (index >= 0);
         }
         else
           positions.push(position);
 
         for (let j = positions.length - 1; j >= 0; j--)
         {
           let currentPosition = positions[j];
-          if (currentSubscription.filters[currentPosition] == filter)
+          let currentFilter = currentSubscription.filterAt(currentPosition);
+          if (currentFilter && currentFilter.text == filter.text)
           {
-            currentSubscription.filters.splice(currentPosition, 1);
-            if (currentSubscription.filters.indexOf(filter) < 0)
+            currentSubscription.deleteFilterAt(currentPosition);
+            if (currentSubscription.searchFilter(filter) < 0)
               filter.removeSubscription(currentSubscription);
             filterNotifier.emit("filter.removed", filter, currentSubscription,
                                 currentPosition);
           }
         }
       }
     }
   }
 
   /**
    * Moves a user-defined filter to a new position.
    * @param {Filter} filter
    * @param {SpecialSubscription} subscription The subscription where the
    *   filter is located.
    * @param {number} oldPosition The current position of the filter.
    * @param {number} newPosition The new position of the filter.
    */
   moveFilter(filter, subscription, oldPosition, newPosition)
   {
-    if (!(subscription instanceof SpecialSubscription) ||
-        subscription.filters[oldPosition] != filter)
-    {
+    if (!(subscription instanceof SpecialSubscription))
       return;
-    }
+
+    let currentFilter = subscription.filterAt(oldPosition);
+    if (!currentFilter || currentFilter.text != filter.text)
+      return;
 
     newPosition = Math.min(Math.max(newPosition, 0),
-                           subscription.filters.length - 1);
+                           subscription.filterCount - 1);
     if (oldPosition == newPosition)
       return;
 
-    subscription.filters.splice(oldPosition, 1);
-    subscription.filters.splice(newPosition, 0, filter);
+    subscription.deleteFilterAt(oldPosition);
+    subscription.insertFilterAt(filter, newPosition);
     filterNotifier.emit("filter.moved", filter, subscription, oldPosition,
                         newPosition);
   }
 
   /**
    * Increases the hit count for a filter by one.
    * @param {Filter} filter
    */
   increaseHitCount(filter)
   {
@@ skipping 45 matching lines @@
         let knownSubscriptions = new Map();
         for (let subscription of parser.subscriptions)
           knownSubscriptions.set(subscription.url, subscription);
 
         this.fileProperties = parser.fileProperties;
         this.knownSubscriptions = knownSubscriptions;
         Filter.knownFilters = parser.knownFilters;
         Subscription.knownSubscriptions = parser.knownSubscriptions;
 
         if (!silent)
+        {
           filterNotifier.emit("load");
+
+          // Clear any in-memory caches that may have been created during
+          // initialization.
+          clearSubscriptionCaches();
+        }
       }
     };
   }
 
   /**
    * Loads all subscriptions from disk.
    * @returns {Promise} A promise resolved or rejected when loading is complete.
    */
   loadFromDisk()
   {
@@ skipping 28 matching lines @@
         }
       });
     }).catch(error =>
     {
       Cu.reportError(error);
       return tryBackup(1);
     }).then(() =>
     {
       this.initialized = true;
       filterNotifier.emit("load");
+      clearSubscriptionCaches();
     });
   }
 
   /**
    * Constructs the file name for a <code>patterns.ini</code> backup.
    * @param {number} backupIndex Number of the backup file (1 being the most
    *   recent).
    * @returns {string} Backup file name.
    */
   getBackupName(backupIndex)
@@ skipping 27 matching lines @@
    * @yields {string}
    */
   *exportData()
   {
     // Do not persist external subscriptions
     let subscriptions = [];
     for (let subscription of this.subscriptions())
     {
       if (!(subscription instanceof ExternalSubscription) &&
           !(subscription instanceof SpecialSubscription &&
-            subscription.filters.length == 0))
+            subscription.filterCount == 0))
       {
         subscriptions.push(subscription);
       }
     }
 
     yield "# Adblock Plus preferences";
     yield "version=" + this.formatVersion;
 
     let saved = new Set();
 
     // Save subscriptions
     for (let subscription of subscriptions)
     {
       yield* subscription.serialize();
       yield* subscription.serializeFilters();
     }
 
     // Save filter data
     for (let subscription of subscriptions)
     {
-      for (let filter of subscription.filters)
+      for (let filter of subscription.filters())
       {
         if (!saved.has(filter.text))
         {
           yield* filter.serialize();
           saved.add(filter.text);
         }
       }
     }
   }
 
@@ skipping 126 matching lines @@
 
 /**
  * Reads the user's filters from disk, manages them in memory, and writes them
  * back to disk.
  */
 let filterStorage = new FilterStorage();
 
 exports.filterStorage = filterStorage;
 
 /**
+ * Adds to a filter any subscriptions in the storage to which the filter
+ * belongs.
+ * @param {Filter} filter
+ */
+function addSubscriptionsToFilter(filter)
+{
+  for (let subscription of filterStorage.subscriptions())
+  {
+    if (subscription.hasFilter(filter))
+      filter.addSubscription(subscription);
+  }
+
+  filter.subscriptionsAdded = true;
+}
+
+/**
  * Connects a subscription to its filters without any notifications.
  * @param {Subscription} subscription The subscription that should be
  *   connected to its filters.
- */
-function connectSubscriptionFilters(subscription)
+ * @param {?Array.<Filter>} [filters] A list of filters to which the
+ *   subscription should be connected. If this is not given, the subscription
+ *   is connected to its own filters.
+ */
+function connectSubscriptionFilters(subscription, filters)
 {
   if (!filterStorage.knownSubscriptions.has(subscription.url))
     return;
 
-  for (let filter of subscription.filters)
+  for (let filter of filters || subscription.filters())
     filter.addSubscription(subscription);
 }
 
 /**
  * Disconnects a subscription from its filters without any notifications.
  * @param {Subscription} subscription The subscription that should be
  *   disconnected from its filters.
- */
-function disconnectSubscriptionFilters(subscription)
+ * @param {?Array.<Filter>} [filters] A list of filters from which the
+ *   subscription should be disconnected. If this is not given, the
+ *   subscription is disconnected from its own filters.
+ */
+function disconnectSubscriptionFilters(subscription, filters)
 {
   if (!filterStorage.knownSubscriptions.has(subscription.url))
     return;
 
-  for (let filter of subscription.filters)
+  for (let filter of filters || subscription.filters())
     filter.removeSubscription(subscription);
 }
+
+/**
+ * Clears any in-memory caches held by subscriptions in the storage.
+ */
+function clearSubscriptionCaches()
+{
+  for (let subscription of filterStorage.subscriptions())
+    subscription.clearCaches();
+}