Issue 7016 - Convert serialization functions into generators

lib/filterStorage.js

Index: lib/filterStorage.js
===================================================================
--- a/lib/filterStorage.js
+++ b/lib/filterStorage.js
@@ -454,46 +454,38 @@
         subscriptions.push(subscription);
       }
     }
 
     yield "# Adblock Plus preferences";
     yield "version=" + formatVersion;
 
     let saved = new Set();
-    let buf = [];
 
     // Save subscriptions
     for (let subscription of subscriptions)
     {
-      yield "";

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

I think we still need the blank line.

[Manish Jethani, 2018/10/04 04:50:11]

Oh, it seems blank lines are really not necessary, the tests seem to pass for
me. Can you look into `lib/iniParser.js` to confirm that this is the case?
(Note: just because the tests pass doesn't mean it's backwards compatible
behavior, we'll have to make sure we can still read any patterns.ini files
generated by the current production version of this code.)

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

I thought that this was iniparser processes each line independently and if it
receives a blank line it basically acts like a pass and doesn't do anything.
Perhaps I am mistaken?

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

Yeah, blank lines are ignored [1]

[1]:
https://github.com/adblockplus/adblockpluscore/blob/9fba045086ff73e3986fac2fe...

+      yield* subscription.serialize();
 
-      subscription.serialize(buf);
       if (subscription.filters.length)

[Manish Jethani, 2018/10/04 04:08:59]

One thing worth noting here as that before this change the serialization of a
subscription was an atomic operation. This means the entire subscription was
added to a buffer and _then_ each entry was yielded out. After this change, it
would be possible for the subscription to be modified while it is being
serialized. This could have POTENTIALLY DISASTROUS consequences, like the
generation of a patterns.ini file that the INI parser is unable to read (thus
causing Adblock Plus to no longer function).

I'd like to take my time to think about this, but at the very least it, since it
is now possible for the number of filters to go from non-zero to zero between
this length check here and the actual serialization of the filters, this is
clearly redundant. We can remove this check here. At the same time, let's make
sure that INI parser is able to read a file with a "[Subscription filters]"
section containing _no_ filters.

In general we really, really, really have to make sure that this function is
never able to generate a patterns.ini that the INI parser is unable to parse.

[Manish Jethani, 2018/10/04 04:17:06]

Additionally, it should never be possible for this function to generate a
patterns.ini that the INI parser can parse successfully but that still results
in objects (subscriptions and filters) that are in an _inconsistent_ state
(which may cause errors elsewhere).

So this change is not as simple as it may look like on the surface. We are
definitely complicating things here, but I think we could do this in a proper
way.

We will definitely have to add some more tests.

[Manish Jethani, 2018/10/04 04:36:23]

Alright, some more thinking about this. Let's do this in two steps. In the first
step, let's do this:

  yield [...subscription.serialize()];

Instead of this:

  yield* subscription.serialize();

This cancels out some of the benefit of this change, but it still makes the code
more readable and I have a hunch that it will still perform better.

In the next step (another patch, another time), let's carefully evaluate the
consequences of generating an object's serialized representation line by line.

So let's not change the behavior of the `exportData` function as part of this
patch.

Now based on everything above, we should roll the "[Subscription filters]" part
into `Subscription.prototype.serialize`, which can take an optional
`withFilters` (boolean) parameter and do this part only if it is true (it should
be false for `Subscription.prototype.toString`).

[Manish Jethani, 2018/10/04 05:10:06]

Sorry, I mean `yield*` here.

[Manish Jethani, 2018/10/04 06:10:15]

I profiled this. It performs slightly better than what we have right now.

If we change it to `yield* subscription.serialize()` then it performs
significantly better. Again, not surprising, but it's good to verify this.

If you'd like to try it out, reload the extension, open the DevTools for the
background page, go to the Performance tab and start profiling. Then run this
script, which will open up the Alexa Top 50:

  #!/bin/bash
  for domain in $(curl https://www.alexa.com/topsites | sed -nE
's/.*\/siteinfo\/([^"]*).*/\1/p')
  do
    open -a 'Google Chrome' "http://$domain"
  done

Stop profiling after approximately one minute, then go to the Bottom-up tab at
the bottom of the window (default layout) and search for "exportData".

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

So, I had similar concerns. But figured we can hash it out here, and this
version was more 'to spec' in my mind.

I actually agree that this is a pretty huge change, however doing the change
which is slightly better performance wise with plans to later make the larger
change seems like it may be more of a delay than anything since we would still
need to address the same issue we face here in the future. So, I think it's
worth it to just take the time to test and properly implement the full change. 

However, I do also realize this change makes the code a bit harder to follow and
so we should consider how much that matters here. We are trying to optimize core
as much as possible (for good reason) and as long as it makes sense, then I am
up for it.

Also, if you insist we just hold off for now and use your suggestion I am
perfectly happy to do so, this was just my two rupees worth of opinion.

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

Alright, let's do this properly.

So the strategy is to take a snapshot of the object right at the beginning of
the serialize function, which makes it just like the current implementation in
terms of serializing a consistent state of the object.

  let {prop1, prop2, prop3} = this;
  if (prop1)
    yield prop1;
  if (prop2)
    yield prop2;
  if (prop3)
    yield prop3;

This way, if the value of a property changes in between yield statements, it
would still yield the old value.

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

Spent an embarrassing amount of time trying to figure out what to change here
but realized this reply is probably meant to describe the yield approach (which
is addressed)

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

Ack.

Yeah, this was just a description.

       {
-        buf.push("", "[Subscription filters]");

[Manish Jethani, 2018/10/04 04:08:59]

We would have to yield a blank line here before yielding "[Subscription
filters]"

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

See my comment above :)

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

Acknowledged.

-        subscription.serializeFilters(buf);
+        yield "[Subscription filters]";
+        yield* subscription.serializeFilters();
       }
-      for (let line of buf)
-        yield line;
-      buf.splice(0);
     }
 
     // Save filter data
     for (let subscription of subscriptions)
     {
       for (let filter of subscription.filters)
       {
         if (!saved.has(filter.text))
         {
-          filter.serialize(buf);
+          yield* filter.serialize();
           saved.add(filter.text);
-          for (let line of buf)
-            yield line;
-          buf.splice(0);
         }
       }
     }
   },
 
   /**
    * Will be set to true if saveToDisk() is running (reentrance protection).
    * @type {boolean}