Issue 6856 - Remove FilterStorage.moveSubscription

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 72 matching lines @@
    * Yields subscriptions containing all filters
    * @yields {Subscription}
    */
   *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.

+   * @type {number}
    */
   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.knownSubscriptions.values())
     {
       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
         if (!generalSubscription &&
             (!subscription.defaults || !subscription.defaults.length))
@@ skipping 237 matching lines @@
   /**
    * Loads all subscriptions from the disk.
    * @return {Promise} promise resolved or rejected when loading is complete
    */
   loadFromDisk()
   {
     let tryBackup = backupIndex =>
     {
       return this.restoreBackup(backupIndex, true).then(() =>
       {
-        if (this.subscriptionCount == 0)

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

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

+        if (this.knownSubscriptions.size == 0)
           return tryBackup(backupIndex + 1);
       }).catch(error =>
       {
         // Give up
       });
     };
 
     return IO.statFile(this.sourceFile).then(statData =>
     {
       if (!statData.exists)
       {
         this.firstRun = true;
         return;
       }
 
       let parser = this.importData(true);
       return IO.readFromFile(this.sourceFile, parser).then(() =>
       {
         parser(null);
-        if (this.subscriptionCount == 0)
+        if (this.knownSubscriptions.size == 0)
         {
           // No filter subscriptions in the file, this isn't right.
           throw new Error("No data in the file");
         }
       });
     }).catch(error =>
     {
       Cu.reportError(error);
       return tryBackup(1);
     }).then(() =>
@@ skipping 33 matching lines @@
       return this.saveToDisk();
     });
   },
 
   /**
    * Generator serializing filter data and yielding it line by line.
    */
   *exportData()
   {
     // Do not persist external subscriptions
-    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)
-    );
+    let subscriptions = [];
+    for (let subscription of this.subscriptions())
+    {
+      if (!(subscription instanceof ExternalSubscription))
+        subscriptions.push(subscription);
+    }
 
     yield "# Adblock Plus preferences";
     yield "version=" + formatVersion;
 
     let saved = new Set();
     let buf = [];
 
     // Save subscriptions
     for (let subscription of subscriptions)
     {
@@ skipping 187 matching lines @@
  * @param {Subscription} subscription filter subscription to be removed
  */
 function removeSubscriptionFilters(subscription)
 {
   if (!FilterStorage.knownSubscriptions.has(subscription.url))
     return;
 
   for (let filter of subscription.filters)
     filter.removeSubscription(subscription);
 }