Issue 7016 - Convert serialization functions into generators

lib/subscriptionClasses.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 107 matching lines @@
     if (value != this._disabled)
     {
       let oldValue = this._disabled;
       this._disabled = value;
       filterNotifier.emit("subscription.disabled", this, value, oldValue);
     }
     return this._disabled;
   },
 
   /**
-   * Serializes the subscription to an array of strings for writing
-   * out on the disk.
+   * Serializes the subscription for writing out on disk.
    * @yields {string}
    */
   *serialize()
   {
+    let {url, type, _title, _fixedTitle, _disabled} = this;
+
     yield "[Subscription]";

[Manish Jethani, 2018/10/09 15:11:04]

Let's extract the values first:

  let {url, type, _title, _fixedTitle, _disabled} = this;

This gives us a "snapshot" of the object at the time the function is first
called, which means we always serialize a consistent state of the object.

[Jon Sonesen, 2018/10/12 03:50:06]

Done.

-    yield "url=" + this.url;
-
-    if (this.type)
-      yield "type=" + this.type;
-    if (this._title)
-      yield "title=" + this._title;
-    if (this._fixedTitle)
+    yield "url=" + url;
+
+    if (type)
+      yield "type=" + type;
+    if (_title)
+      yield "title=" + _title;
+    if (_fixedTitle)
       yield "fixedTitle=true";
-    if (this._disabled)
+    if (_disabled)
       yield "disabled=true";
   },
 
   *serializeFilters()

[Manish Jethani, 2018/10/04 03:37:12]

This might be a good place to introduce a generic map function, as described in
https://issues.adblockplus.org/ticket/6434

  function* map(iterable, callback, thisArg)
  {
    for (let item of iterable)
      yield callback.call(thisArg, item);
  }

We could put this is in `lib/coreUtils.js`.

Then this function would look like this:

  *serializeFilters()
  {
    yield* map(this.filters, ({text}) => text.replace(/\[/g, "\\["));
  }

On the other hand, the whole point of this exercise is to improve the
performance. The reason I think it might be OK to do this here is that we're
already calling `String.prototype.replace` so it's just an additional function
call in the stack, but if you decide to go down this route you would have to
profile this to make sure at least it doesn't perform any worse.

You could also just forget about it since it's not exactly related to the change
we're trying to make here (it's just a thought I had so I thought I'd share it
in this context).

[Jon Sonesen, 2018/10/06 00:06:34]

This most likely will turn into a rather large change, and I am pretty
indifferent here. I think I am leaning towards not worrying about it here since
it will be already a lot of changes. But I will still profile it, if it's not a
huge difference I still might go for it.

[Manish Jethani, 2018/10/09 15:11:04]

Acknowledged.

   {
-    for (let filter of this.filters)
+    let {filters} = this;
+
+    yield "[Subscription filters]";
+
+    for (let filter of filters)
       yield filter.text.replace(/\[/g, "\\[");
   },
 
   toString()
   {
     return [...this.serialize()].join("\n");
   }
 };
 
 /**
@@ skipping 115 matching lines @@
 
     return false;
   },
 
   /**
    * See Subscription.serialize()
    * @inheritdoc
    */
   *serialize()
   {
+    let {defaults, _lastDownload} = this;
+
     yield* Subscription.prototype.serialize.call(this);

[Manish Jethani, 2018/10/09 15:11:04]

Let's extract the values first:

  let {defaults, _lastDownload} = this;

(This should be the first line, even before calling the superclass's serialize
method)

[Jon Sonesen, 2018/10/12 03:50:06]

Done.

-    if (this.defaults && this.defaults.length)
+
+    if (defaults)
     {
       yield "defaults=" +

[Manish Jethani, 2018/10/04 03:37:12]

Nit: Since there is no longer a function call here, we should align the string
concatenation like so:

  yield "defaults=" + 
        this.defaults.filter(
          type => SpecialSubscription.defaultsMap.has(type)
        ).join(" ");

Also it might be nice to get rid of the call to `Array.prototype.filter` here
(but then again we might be getting rid of default subscription groups anyway,
see https://issues.adblockplus.org/ticket/6859).

[Jon Sonesen, 2018/10/06 00:06:34]

Acknowledged.

-        this.defaults.filter(
-          type => SpecialSubscription.defaultsMap.has(type)
-        ).join(" ")
-      ;
-    }
-    if (this._lastDownload)
-      yield "lastDownload=" + this._lastDownload;
+            defaults.filter(
+              type => SpecialSubscription.defaultsMap.has(type)
+            ).join(" ");
+    }
+    if (_lastDownload)
+      yield "lastDownload=" + _lastDownload;
   }
 });
 
 SpecialSubscription.defaultsMap = new Map([
   ["whitelist", WhitelistFilter],
   ["blocking", BlockingFilter],
   ["elemhide", ElemHideBase]
 ]);
 
 /**
@@ skipping 87 matching lines @@
     }
     return this._lastDownload;
   },
 
   /**
    * See Subscription.serialize()
    * @inheritdoc
    */
   *serialize()
   {
+    let {_homepage, _lastDownload} = this;
+
     yield* Subscription.prototype.serialize.call(this);

[Manish Jethani, 2018/10/09 15:11:04]

Same thing, let's extract _homepage and _lastDownload as the first thing.

[Jon Sonesen, 2018/10/12 03:50:06]

Done.

-    if (this._homepage)
-      yield "homepage=" + this._homepage;
-    if (this._lastDownload)
-      yield "lastDownload=" + this._lastDownload;
+
+    if (_homepage)
+      yield "homepage=" + _homepage;
+    if (_lastDownload)
+      yield "lastDownload=" + _lastDownload;
   }
 });
 
 /**
  * Class for filter subscriptions updated externally (by other extension)
  * @param {string} url    see {@link Subscription Subscription()}
  * @param {string} [title]  see {@link Subscription Subscription()}
  * @constructor
  * @augments RegularSubscription
  */
 function ExternalSubscription(url, title)
 {
   RegularSubscription.call(this, url, title);
 }
 exports.ExternalSubscription = ExternalSubscription;
 
 ExternalSubscription.prototype = extend(RegularSubscription, {
   /**
    * See Subscription.serialize()
    * @inheritdoc
    */
-  *serialize()
-  {
-    yield new Error(

[Manish Jethani, 2018/10/04 03:37:12]

This would still be a throw.

[Manish Jethani, 2018/10/04 05:30:00]

I see, you're getting an error from ESLint. I hate ESLint.

You can just add the following above the `throw` statement:

  // This is just to make ESLint happy.
  yield;

[Jon Sonesen, 2018/10/12 03:50:06]

Inline disable ok?

[Manish Jethani, 2018/10/14 20:05:36]

Yeah, that's probably better.

+  *serialize() // eslint-disable-line require-yield
+  {
+    throw new Error(
       "Unexpected call, external subscriptions should not be serialized"
     );
   }
 });
 
 /**
  * Class for filter subscriptions updated externally (by other extension)
  * @param {string} url  see {@link Subscription Subscription()}
  * @param {string} [title]  see {@link Subscription Subscription()}
  * @constructor
@@ skipping 104 matching lines @@
    * @type {number}
    */
   downloadCount: 0,
 
   /**
    * See Subscription.serialize()
    * @inheritdoc
    */
   *serialize()
   {
+    let {downloadStatus, lastSuccess, lastCheck, expires,
+         softExpiration, errors, version, requiredVersion,
+         downloadCount} = this;
+
     yield* RegularSubscription.prototype.serialize.call(this);

[Manish Jethani, 2018/10/09 15:11:04]

Same thing, let's extract all the values of the properties here first before we
ever yield anything.

[Jon Sonesen, 2018/10/12 03:50:06]

Done.

-    if (this.downloadStatus)
-      yield "downloadStatus=" + this.downloadStatus;
-    if (this.lastSuccess)
-      yield "lastSuccess=" + this.lastSuccess;
-    if (this.lastCheck)
-      yield "lastCheck=" + this.lastCheck;
-    if (this.expires)
-      yield "expires=" + this.expires;
-    if (this.softExpiration)
-      yield "softExpiration=" + this.softExpiration;
-    if (this.errors)
-      yield "errors=" + this.errors;
-    if (this.version)
-      yield "version=" + this.version;
-    if (this.requiredVersion)
-      yield "requiredVersion=" + this.requiredVersion;
-    if (this.downloadCount)
-      yield "downloadCount=" + this.downloadCount;
+
+    if (downloadStatus)
+      yield "downloadStatus=" + downloadStatus;
+    if (lastSuccess)
+      yield "lastSuccess=" + lastSuccess;
+    if (lastCheck)
+      yield "lastCheck=" + lastCheck;
+    if (expires)
+      yield "expires=" + expires;
+    if (softExpiration)
+      yield "softExpiration=" + softExpiration;
+    if (errors)
+      yield "errors=" + errors;
+    if (version)
+      yield "version=" + version;
+    if (requiredVersion)
+      yield "requiredVersion=" + requiredVersion;
+    if (downloadCount)
+      yield "downloadCount=" + downloadCount;
   }
 });