Issue 7029 - Remove subscriptions property of Filter object

lib/filterStorage.js

Index: lib/filterStorage.js
===================================================================
--- a/lib/filterStorage.js
+++ b/lib/filterStorage.js
@@ -105,22 +105,36 @@
    * @type {string}
    */
   get sourceFile()
   {
     return "patterns.ini";
   }
 
   /**
-   * Yields all subscriptions in the storage.
+   * Yields subscriptions in the storage.
+   * @param {?string} [filterText] The filter text for which to look. If
+   *   specified, the function yields only those subscriptions that contain the
+   *   given filter text. By default the function yields all subscriptions.
    * @yields {Subscription}
    */
-  *subscriptions()
+  *subscriptions(filterText = null)
   {
-    yield* this.knownSubscriptions.values();
+    if (filterText == null)
+    {
+      yield* this.knownSubscriptions.values();
+    }
+    else
+    {
+      for (let subscription of this.knownSubscriptions.values())
+      {
+        if (subscription.hasFilterText(filterText))
+          yield subscription;
+      }
+    }
   }
 
   /**
    * The number of subscriptions in the storage.
    * @type {number}
    */
   get subscriptionCount()
   {
@@ -160,32 +174,29 @@
    * @param {Subscription} subscription The subscription to be added.
    */
   addSubscription(subscription)
   {
     if (this.knownSubscriptions.has(subscription.url))
       return;
 
     this.knownSubscriptions.set(subscription.url, subscription);
-    connectSubscriptionFilters(subscription);
 
     filterNotifier.emit("subscription.added", subscription);
   }
 
   /**
    * 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);
   }
@@ -193,41 +204,38 @@
   /**
    * Replaces the list of filters in a subscription with a new list.
    * @param {Subscription} subscription The subscription to be updated.
    * @param {Array.<string>} filterText The new filter text.
    */
   updateSubscriptionFilters(subscription, filterText)
   {
     let oldFilterText = [...subscription.filterText()];
-    disconnectSubscriptionFilters(subscription, oldFilterText);

[Manish Jethani, 2019/02/26 12:29:57]

You might think that looking up the subscriptions for a filter makes this more
expensive, but actually since we no longer have to disconnect/connect filters
from/to their subscriptions, this is actually faster in practice as long as you
have a reasonable number of subscriptions (I tried with 5).

[hub, 2019/03/08 21:36:19]

my default config has 6. Add one language, that make 7. And then if I remember,
each user filter is a special subscription.

[Manish Jethani, 2019/03/30 20:50:35]

Each user-defined filter is not a special subscription, only each type
(blocking, hiding, etc.) is a special subscription. Actually there are only two
types: blocking and hiding.

I have the following lists:

 1.  Adblock Warning Removal List
 2.  ABP filters
 3.  EasyPrivacy
 4.  Fanboy's Social Blocking List
 5.  EasyList Germany+EasyList
 6.  EasyList China+EasyList
 7.  Allow nonintrusive advertising
 8.  EasyList

I added a `console.time(subscription.title)` and
`console.timeEnd(subscription.title)` to the start and end of this function
respectively. And then I manually clicked on the "Update all filter lists"
button on the options page.

Here's what I got:

Before (3 runs):

  Adblock Warning Removal List: 2.574951171875ms
  ABP filters: 2.102294921875ms
  EasyPrivacy: 121.102294921875ms
  Fanboy's Social Blocking List: 85.465087890625ms
  EasyList Germany+EasyList: 343.808837890625ms
  EasyList China+EasyList: 382.23095703125ms
  EasyList: 231.58203125ms
  Allow nonintrusive advertising: 170.242919921875ms

  ABP filters: 2.664794921875ms
  Adblock Warning Removal List: 2.403076171875ms
  Fanboy's Social Blocking List: 39.18603515625ms
  EasyPrivacy: 47.486083984375ms
  EasyList: 378.468994140625ms
  EasyList Germany+EasyList: 389.359130859375ms
  EasyList China+EasyList: 364.27099609375ms
  Allow nonintrusive advertising: 62.0830078125ms

  Adblock Warning Removal List: 1.76513671875ms 
  ABP filters: 2.052978515625ms
  EasyPrivacy: 45.468994140625ms
  Fanboy's Social Blocking List: 47.925048828125ms
  EasyList Germany+EasyList: 331.406982421875ms
  EasyList China+EasyList: 462.910888671875ms
  EasyList: 313.959228515625ms
  Allow nonintrusive advertising: 164.7099609375ms

After (3 runs):

  Adblock Warning Removal List: 3.661865234375ms
  ABP filters: 26.75830078125ms
  EasyPrivacy: 153.299072265625ms
  Fanboy's Social Blocking List: 101.579833984375ms
  EasyList Germany+EasyList: 298.937744140625ms
  EasyList China+EasyList: 368.741943359375ms
  Allow nonintrusive advertising: 65.531005859375ms
  EasyList: 289.38916015625ms

  Adblock Warning Removal List: 2.134765625ms
  ABP filters: 2.312255859375ms
  EasyPrivacy: 90.31005859375ms
  Fanboy's Social Blocking List: 76.914794921875ms
  EasyList Germany+EasyList: 327.611083984375ms
  EasyList: 305.14697265625ms
  EasyList China+EasyList: 358.02294921875ms
  Allow nonintrusive advertising: 66.77587890625ms

  Adblock Warning Removal List: 1.616943359375ms
  ABP filters: 2.01611328125ms
  EasyPrivacy: 60.262939453125ms
  Fanboy's Social Blocking List: 64.0009765625ms
  EasyList: 292.301025390625ms
  EasyList China+EasyList: 342.841064453125ms
  EasyList Germany+EasyList: 315.51220703125ms
  Allow nonintrusive advertising: 137.76318359375ms

It is comparable or better.

Let's remember a few things:

 1.  This happens in the background, at most once per hour
 2.  We can optimize this by looking only at the delta between old and new
filters (though this optimization is not necessary for this patch)
 3.  We are going to switch to incremental filter list updates, which will give
us a delta anyway

I also checked the load time by adding `console.time("Load")` and
`console.timeEnd("Load")` in the `importData()` function in
`lib/filterStorage.js` and reloading the extension multiple times.

Before (3 runs):

  Load: 1321.534912109375ms
  Load: 1121.9091796875ms
  Load: 1387.422607421875ms

After (3 runs):

  Load: 774.85986328125ms
  Load: 620.704833984375ms
  Load: 648.51416015625ms

This patch significantly speeds up loading.

     subscription.clearFilters();
 
     for (let text of filterText)
       subscription.addFilterText(text);
 
-    connectSubscriptionFilters(subscription, filterText);
-
     filterNotifier.emit("subscription.updated", subscription, oldFilterText);
   }
 
   /**
    * 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.subscriptions(filter.text))
       {
         if (currentSubscription instanceof SpecialSubscription &&
             !currentSubscription.disabled)
         {
           return;   // No need to add
         }
       }
       subscription = this.getGroupForFilter(filter);
@@ -238,17 +246,16 @@
       subscription = SpecialSubscription.createForFilter(filter);
       this.addSubscription(subscription);
       return;
     }
 
     if (typeof position == "undefined")
       position = subscription.filterCount;
 
-    filter.addSubscription(subscription);
     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
@@ -256,46 +263,44 @@
    *   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.subscriptions(filter.text)
     );
     for (let currentSubscription of subscriptions)
     {
       if (currentSubscription instanceof SpecialSubscription)
       {
         let positions = [];
         if (typeof position == "undefined")
         {
           let index = -1;
           do
           {
-            index = currentSubscription.searchFilter(filter, index + 1);
+            index = currentSubscription.findFilterIndex(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];
           let currentFilterText =
             currentSubscription.filterTextAt(currentPosition);
           if (currentFilterText && currentFilterText == filter.text)
           {
             currentSubscription.deleteFilterAt(currentPosition);
-            if (currentSubscription.searchFilter(filter) < 0)
-              filter.removeSubscription(currentSubscription);
             filterNotifier.emit("filter.removed", filter, currentSubscription,
                                 currentPosition);
           }
         }
       }
     }
   }
 
@@ -645,42 +650,8 @@
 
 /**
  * Reads the user's filters from disk, manages them in memory, and writes them
  * back to disk.
  */
 let filterStorage = new FilterStorage();
 
 exports.filterStorage = filterStorage;
-
-/**
- * Connects a subscription to its filters without any notifications.
- * @param {Subscription} subscription The subscription that should be
- *   connected to its filters.
- * @param {?Array.<string>} [filterText] A list of filters (in text form) to
- *   which the subscription should be connected. If this is not given, the
- *   subscription is connected to its own filters.
- */
-function connectSubscriptionFilters(subscription, filterText)
-{
-  if (!filterStorage.knownSubscriptions.has(subscription.url))
-    return;
-
-  for (let text of filterText || subscription.filterText())
-    Filter.fromText(text).addSubscription(subscription);
-}
-
-/**
- * Disconnects a subscription from its filters without any notifications.
- * @param {Subscription} subscription The subscription that should be
- *   disconnected from its filters.
- * @param {?Array.<string>} [filterText] A list of filters (in text form) from
- *   which the subscription should be disconnected. If this is not given, the
- *   subscription is disconnected from its own filters.
- */
-function disconnectSubscriptionFilters(subscription, filterText)
-{
-  if (!filterStorage.knownSubscriptions.has(subscription.url))
-    return;
-
-  for (let text of filterText || subscription.filterText())
-    Filter.fromText(text).removeSubscription(subscription);
-}