Issue 7324 - Simplify logic for domains property

lib/filterClasses.js

Index: lib/filterClasses.js
===================================================================
--- a/lib/filterClasses.js
+++ b/lib/filterClasses.js
@@ -19,16 +19,17 @@
 
 /**
  * @fileOverview Definition of Filter class and its subclasses.
  */
 
 const {extend} = require("./coreUtils");
 const {filterToRegExp} = require("./common");
 const {normalizeHostname, domainSuffixes} = require("./url");
+const {Cache} = require("./caching");
 const {filterNotifier} = require("./filterNotifier");
 
 const resources = require("../data/resources.json");
 
 /**
  * Map of internal resources for URL rewriting.
  * @type {Map.<string,string>}
  */
@@ -47,20 +48,24 @@
  * Regular expression used to match the <code>^</code> suffix in an otherwise
  * literal pattern.
  * @type {RegExp}
  */
 // Note: This should match the pattern in lib/common.js
 let separatorRegExp = /[\x00-\x24\x26-\x2C\x2F\x3A-\x40\x5B-\x5E\x60\x7B-\x7F]/;
 
 /**
- * All known unique domain sources mapped to their parsed values.
- * @type {Map.<string,Map.<string,boolean>>}
+ * Cache of domain maps. The domains part of filter text
+ * (e.g. <code>example.com,~mail.example.com</code>) is often repeated across
+ * filters. This cache enables deduplication of the <code>Map</code> objects
+ * that specify on which domains the filter does and does not apply, which
+ * reduces memory usage and improves performance.
+ * @type {Map.<string, Map.<string, boolean>>}
  */
-let knownDomainMaps = new Map();
+let domainMapCache = new Cache(1000);
 
 /**
  * Checks whether the given pattern is a string of literal characters with no
  * wildcards or any other special characters. If the pattern is prefixed with a
  * <code>||</code> or suffixed with a <code>^</code> but otherwise contains no
  * special characters, it is still considered to be a literal pattern.
  * @param {string} pattern
  * @returns {boolean}
@@ -402,17 +407,17 @@
  * @constructor
  * @augments Filter
  */
 function ActiveFilter(text, domains)
 {
   Filter.call(this, text);
 
   if (domains)
-    this.domainSource = domains;
+    this.domainSource = domains.toLowerCase();

[Manish Jethani, 2019/03/04 12:02:54]

Lower-case in advance during initialization.

[hub, 2019/03/06 13:10:40]

Do we know if there is a performance difference between domains already being
all lowercase and not? Whether it is worth, in addition to this, to ensure the
filters (lists) are normalized.

[Manish Jethani, 2019/03/06 18:11:10]

You mean that we could require that the domains are already lower-cased in the
filter list so we don't have to lower-case them? I don't know if it would make
much of a difference to the loading of the filters. I also do expect that we
could make lots of other improvements that would give us a much bigger speed
boost for the initial loading of the filters.

[hub, 2019/03/06 18:14:25]

I was just wondering if toLowerCase() is faster when fed already lowercase
strings, and in that case if that would be worth it for the filter lists to be
normalized. Just an idea here.

[Manish Jethani, 2019/03/06 18:28:10]

Ah, I see what you mean. I don't expect it to be faster for strings that are
already lower-cased. If that were the case, we wouldn't benefit from the change
in changeset 0c5fe8fe506f.

 }
 exports.ActiveFilter = ActiveFilter;
 
 ActiveFilter.prototype = extend(Filter, {
   _disabled: false,
   _hitCount: 0,
   _lastHit: 0,
 
@@ -489,95 +494,67 @@
 
   /**
    * Map containing domains that this filter should match on/not match
    * on or null if the filter should match on all domains
    * @type {?Map.<string,boolean>}
    */
   get domains()
   {
-    let domains = null;
+    let {domainSource} = this;

[Manish Jethani, 2019/03/04 12:02:54]

Tip: The diff of this function is smaller with the `-b` flag. I flattened it
(less nesting, multiple return statements).

+    if (!domainSource)
+      return null;
 
-    if (this.domainSource)
-    {
-      // For most filter types this property is accessed only rarely,
-      // especially when the subscriptions are initially loaded. We defer any
-      // caching by default.
-      let cacheDomains = this._cacheDomains;
-
-      let source = this.domainSource.toLowerCase();
+    let domains = domainMapCache.get(domainSource);
+    if (domains)
+      return domains;
 
-      let knownMap = knownDomainMaps.get(source);
-      if (knownMap)
+    let list = domainSource.split(this.domainSeparator);
+    if (list.length == 1 && list[0][0] != "~")
+    {
+      // Fast track for the common one-domain scenario
+      domains = new Map([[list[0], true], ["", false]]);
+    }
+    else
+    {
+      let hasIncludes = false;
+      for (let i = 0; i < list.length; i++)
       {
-        domains = knownMap;
-      }
-      else
-      {
-        let list = source.split(this.domainSeparator);
-        if (list.length == 1 && list[0][0] != "~")
+        let domain = list[i];
+        if (domain == "")
+          continue;
+
+        let include;
+        if (domain[0] == "~")
         {
-          // Fast track for the common one-domain scenario
-          domains = new Map([[list[0], true], ["", false]]);
+          include = false;
+          domain = domain.substring(1);
         }
         else
         {
-          let hasIncludes = false;
-          for (let i = 0; i < list.length; i++)
-          {
-            let domain = list[i];
-            if (domain == "")
-              continue;
-
-            let include;
-            if (domain[0] == "~")
-            {
-              include = false;
-              domain = domain.substring(1);
-            }
-            else
-            {
-              include = true;
-              hasIncludes = true;
-            }
-
-            if (!domains)
-              domains = new Map();
-
-            domains.set(domain, include);
-          }
-
-          if (domains)
-            domains.set("", !hasIncludes);
+          include = true;
+          hasIncludes = true;
         }
 
-        if (!domains || cacheDomains)
-          knownDomainMaps.set(source, domains);
+        if (!domains)
+          domains = new Map();
+
+        domains.set(domain, include);
       }
 
-      if (!domains || cacheDomains)
-      {
-        this.domainSource = null;
-        Object.defineProperty(this, "domains", {value: domains});
-      }
+      if (domains)
+        domains.set("", !hasIncludes);
     }
 
-    this._cacheDomains = true;
+    domainMapCache.set(domainSource, domains);
 
     return domains;
   },
 
   /**
-   * Whether the value of {@link ActiveFilter#domains} should be cached.
-   * @type {boolean}
-   * @private
-   */
-  _cacheDomains: false,
-
-  /**
    * Array containing public keys of websites that this filter should apply to
    * @type {?string[]}
    */
   sitekeys: null,
 
   /**
    * Checks whether this filter is active on a domain.
    * @param {string} [docDomain] domain name of the document that loads the URL