Issue 7016 - Convert serialization functions into generators

lib/subscriptionClasses.js

Index: lib/subscriptionClasses.js
===================================================================
--- a/lib/subscriptionClasses.js
+++ b/lib/subscriptionClasses.js
@@ -122,43 +122,42 @@
       filterNotifier.emit("subscription.disabled", this, value, oldValue);
     }
     return this._disabled;
   },
 
   /**
    * Serializes the subscription to an array of strings for writing
    * out on the disk.
-   * @param {string[]} buffer  buffer to push the serialization results into
+   * @yields {string}
    */
-  serialize(buffer)
+  *serialize()
   {
-    buffer.push("[Subscription]");
-    buffer.push("url=" + this.url);
+    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)
-      buffer.push("type=" + this.type);
+      yield "type=" + this.type;
     if (this._title)
-      buffer.push("title=" + this._title);
+      yield "title=" + this._title;
     if (this._fixedTitle)
-      buffer.push("fixedTitle=true");
+      yield "fixedTitle=true";
     if (this._disabled)
-      buffer.push("disabled=true");
+      yield "disabled=true";
   },
 
-  serializeFilters(buffer)
+  *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)
-      buffer.push(filter.text.replace(/\[/g, "\\["));
+      yield filter.text.replace(/\[/g, "\\[");
   },
 
   toString()
   {
-    let buffer = [];
-    this.serialize(buffer);
-    return buffer.join("\n");
+    return [...this.serialize()].join("\n");
   }
 };
 
 /**
  * Cache for known filter subscriptions, maps URL to subscription objects.
  * @type {Map.<string,Subscription>}
  */
 Subscription.knownSubscriptions = new Map();
@@ -276,29 +275,29 @@
 
     return false;
   },
 
   /**
    * See Subscription.serialize()
    * @inheritdoc
    */
-  serialize(buffer)
+  *serialize()
   {
-    Subscription.prototype.serialize.call(this, buffer);
+    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)
     {
-      buffer.push("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)
-      buffer.push("lastDownload=" + this._lastDownload);
+      yield "lastDownload=" + this._lastDownload;
   }
 });
 
 SpecialSubscription.defaultsMap = new Map([
   ["whitelist", WhitelistFilter],
   ["blocking", BlockingFilter],
   ["elemhide", ElemHideBase]
 ]);
@@ -394,23 +393,23 @@
     }
     return this._lastDownload;
   },
 
   /**
    * See Subscription.serialize()
    * @inheritdoc
    */
-  serialize(buffer)
+  *serialize()
   {
-    Subscription.prototype.serialize.call(this, buffer);
+    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)
-      buffer.push("homepage=" + this._homepage);
+      yield "homepage=" + this._homepage;
     if (this._lastDownload)
-      buffer.push("lastDownload=" + this._lastDownload);
+      yield "lastDownload=" + this._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
@@ -422,19 +421,19 @@
 }
 exports.ExternalSubscription = ExternalSubscription;
 
 ExternalSubscription.prototype = extend(RegularSubscription, {
   /**
    * See Subscription.serialize()
    * @inheritdoc
    */
-  serialize(buffer)
+  *serialize()
   {
-    throw new Error(
+    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.

       "Unexpected call, external subscriptions should not be serialized"
     );
   }
 });
 
 /**
  * Class for filter subscriptions updated externally (by other extension)
  * @param {string} url  see {@link Subscription Subscription()}
@@ -547,31 +546,31 @@
    * @type {number}
    */
   downloadCount: 0,
 
   /**
    * See Subscription.serialize()
    * @inheritdoc
    */
-  serialize(buffer)
+  *serialize()
   {
-    RegularSubscription.prototype.serialize.call(this, buffer);
+    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)
-      buffer.push("downloadStatus=" + this.downloadStatus);
+      yield "downloadStatus=" + this.downloadStatus;
     if (this.lastSuccess)
-      buffer.push("lastSuccess=" + this.lastSuccess);
+      yield "lastSuccess=" + this.lastSuccess;
     if (this.lastCheck)
-      buffer.push("lastCheck=" + this.lastCheck);
+      yield "lastCheck=" + this.lastCheck;
     if (this.expires)
-      buffer.push("expires=" + this.expires);
+      yield "expires=" + this.expires;
     if (this.softExpiration)
-      buffer.push("softExpiration=" + this.softExpiration);
+      yield "softExpiration=" + this.softExpiration;
     if (this.errors)
-      buffer.push("errors=" + this.errors);
+      yield "errors=" + this.errors;
     if (this.version)
-      buffer.push("version=" + this.version);
+      yield "version=" + this.version;
     if (this.requiredVersion)
-      buffer.push("requiredVersion=" + this.requiredVersion);
+      yield "requiredVersion=" + this.requiredVersion;
     if (this.downloadCount)
-      buffer.push("downloadCount=" + this.downloadCount);
+      yield "downloadCount=" + this.downloadCount;
   }
 });