Issue 7324 - Simplify logic for domains property

lib/filterClasses.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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 /**
  * @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>}
  */
 let resourceMap = new Map(
   Object.keys(resources).map(key => [key, resources[key]])
 );
 
 /**
  * Regular expression used to match the <code>||</code> prefix in an otherwise
  * literal pattern.
  * @type {RegExp}
  */
 let doubleAnchorRegExp = new RegExp(filterToRegExp("||") + "$");
 
 /**
  * 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}
  */
 function isLiteralPattern(pattern)
@@ skipping 331 matching lines @@
  *   Domains that the filter is restricted to separated by domainSeparator
  *   e.g. "foo.com|bar.com|~baz.com"
  * @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,
 
   /**
    * Defines whether the filter is disabled
@@ skipping 66 matching lines @@
    */
   domainSeparator: null,
 
   /**
    * 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)
+    let domains = domainMapCache.get(domainSource);
+    if (domains)
+      return domains;
+
+    let list = domainSource.split(this.domainSeparator);
+    if (list.length == 1 && list[0][0] != "~")
     {
-      // 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;
+      // 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++)
+      {
+        let domain = list[i];
+        if (domain == "")
+          continue;
 
-      let source = this.domainSource.toLowerCase();
-
-      let knownMap = knownDomainMaps.get(source);
-      if (knownMap)
-      {
-        domains = knownMap;
-      }
-      else
-      {
-        let list = source.split(this.domainSeparator);
-        if (list.length == 1 && list[0][0] != "~")
+        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
    * @param {string} [sitekey] public key provided by the document
    * @return {boolean} true in case of the filter being active
@@ skipping 847 matching lines @@
 
   /**
    * Script that should be executed
    * @type {string}
    */
   get script()
   {
     return this.body;
   }
 });