Issue 4057 - Further speedup ElemHide.getSelectorsforDomain

lib/elemHide.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 22 matching lines @@
  */
 var filterByKey = [];
 
 /**
  * Lookup table, keys of the filters by filter text
  * @type Object
  */
 var keyByFilter = Object.create(null);
 
 /**
- * Nested lookup table, filter (or false if inactive) by filter text by domain
+ * Indicates whether we are using the getSelectorsForDomain function and
+ * therefore mainting the required filtersByDomain, filtersBySelector and
+ * unconditionalSelectors lookups. (Will be false for Firefox)
+ * @type Boolean
+ */
+var usingGetSelectorsForDomain = !("nsIStyleSheetService" in Ci);
+
+/**
+ * Nested lookup table, filter (or false if inactive) by filter key by domain.
+ * (Only contains filters that aren't unconditionally matched for all domains.)
  * @type Object
  */
 var filtersByDomain = Object.create(null);
 
 /**
- * Indicates whether we are using (and maintaining) the filtersByDomain lookup.
- * (Will be false for Firefox)
- * @type Boolean
+ * Lookup table, filters by selector. (Only contains filters that have a
+ * selector that is unconditionally matched for all domains.)
  */
-var usingFiltersByDomain = !("nsIStyleSheetService" in Ci);
+var filtersBySelector = Object.create(null);
+
+/**
+ * Array of selectors which unconditionally apply to all domains.
+ */
+var unconditionalSelectors = [];
+
+/**
+ * Indicates that the unconditionalSelectors Array needs to be regenerated.
+ */
+var unconditionalSelectorsDirty = false;
 
 /**
  * Object to be used instead when a filter has a blank domains property.
  */
 var defaultDomains = Object.create(null);
 defaultDomains[""] = true;
 
 /**
  * Lookup table, keys are known element hiding exceptions
  * @type Object
@@ skipping 48 matching lines @@
   },
 
   /**
    * Removes all known filters
    */
   clear: function()
   {
     filterByKey = [];
     keyByFilter = Object.create(null);
     filtersByDomain = Object.create(null);
+    filtersBySelector = Object.create(null);
+    unconditionalSelectors = [];
+    unconditionalSelectorsDirty = false;
     knownExceptions = Object.create(null);
     exceptions = Object.create(null);
     ElemHide.isDirty = false;
     ElemHide.unapply();
   },
 
+  _addToFiltersByDomain: function(filter)
+  {
+    let key = keyByFilter[filter.text];
+    let domains = filter.domains || defaultDomains;
+    for (let domain in domains)
+    {
+      let filters = filtersByDomain[domain];
+      if (!filters)
+        filters = filtersByDomain[domain] = Object.create(null);
+
+      if (domains[domain])
+        filters[key] = filter;
+      else
+        filters[key] = false;
+    }
+  },
+
   /**
    * Add a new element hiding filter
    * @param {ElemHideFilter} filter
    */
   add: function(filter)
   {
     if (filter instanceof ElemHideException)
     {
       if (filter.text in knownExceptions)
         return;
 
       let selector = filter.selector;
       if (!(selector in exceptions))
         exceptions[selector] = [];
       exceptions[selector].push(filter);
+
+      if (usingGetSelectorsForDomain)
+      {
+        // If this is the first exception for a previously unconditionally
+        // applied element hiding selector we need to take care to update the
+        // lookups.
+        let unconditionalFilters = filtersBySelector[selector];
+        if (unconditionalFilters)
+        {
+          for (let f of unconditionalFilters)
+            this._addToFiltersByDomain(f);
+          delete filtersBySelector[selector];
+          unconditionalSelectorsDirty = true;
+        }
+      }
+
       knownExceptions[filter.text] = true;
     }
     else
     {
       if (filter.text in keyByFilter)
         return;
 
       let key = filterByKey.push(filter) - 1;
       keyByFilter[filter.text] = key;
 
-      if (usingFiltersByDomain)
+      if (usingGetSelectorsForDomain)
       {
-        let domains = filter.domains || defaultDomains;
-        for (let domain in domains)
+        if (!(filter.domains || filter.selector in exceptions))
         {
-          let filters = filtersByDomain[domain];
-          if (!filters)
-            filters = filtersByDomain[domain] = Object.create(null);
-
-          if (domains[domain])
-            filters[filter.text] = filter;
+          // The new filter's selector is unconditionally applied to all domains
+          let filters = filtersBySelector[filter.selector];
+          if (filters)
+          {
+            filters.push(filter);
+          }
           else
-            filters[filter.text] = false;
+          {
+            filtersBySelector[filter.selector] = [filter];
+            unconditionalSelectorsDirty = true;
+          }
+        }
+        else
+        {
+          // The new filter's selector only applies to some domains
+          this._addToFiltersByDomain(filter);
         }
       }
 
       ElemHide.isDirty = true;
     }
   },
 
   /**
    * Removes an element hiding filter
    * @param {ElemHideFilter} filter
@@ skipping 14 matching lines @@
     else
     {
       if (!(filter.text in keyByFilter))
         return;
 
       let key = keyByFilter[filter.text];
       delete filterByKey[key];
       delete keyByFilter[filter.text];
       ElemHide.isDirty = true;
 
-      if (usingFiltersByDomain)
+      if (usingGetSelectorsForDomain)
       {
-        let domains = filter.domains || defaultDomains;
-        for (let domain in domains)
+        let filters = filtersBySelector[filter.selector];
+        if (filters)
         {
-          let filters = filtersByDomain[domain];
-          if (filters)
-            delete filters[filter.text];
+          if (filters.length > 1)
+          {
+            let index = filters.indexOf(filter);
+            filters.splice(index, 1);
+          }
+          else
+          {
+            delete filtersBySelector[filter.selector];
+            unconditionalSelectorsDirty = true;
+          }
+        }
+        else
+        {
+          let domains = filter.domains || defaultDomains;
+          for (let domain in domains)
+          {
+            let filters = filtersByDomain[domain];
+            if (filters)
+              delete filters[key];
+          }
         }
       }
     }
   },
 
   /**
    * Checks whether an exception rule is registered for a filter on a particular
    * domain.
    */
   getException: function(/**Filter*/ filter, /**String*/ docDomain) /**ElemHideException*/
@@ skipping 191 matching lines @@
   /**
    * Retrieves an element hiding filter by the corresponding protocol key
    */
   getFilterByKey: function(/**String*/ key) /**Filter*/
   {
     return (key in filterByKey ? filterByKey[key] : null);
   },
 
   /**
    * Returns a list of all selectors active on a particular domain, must not be
-   * used in Firefox (when usingFiltersByDomain is false).
+   * used in Firefox (when usingGetSelectorsForDomain is false).
    */
   getSelectorsForDomain: function(/**String*/ domain, /**Boolean*/ specificOnly)
   {
-    if (!usingFiltersByDomain)
+    if (!usingGetSelectorsForDomain)
       throw new Error("getSelectorsForDomain can not be used in Firefox!");
 
-    let selectors = [];
+    if (unconditionalSelectorsDirty)
+    {
+      unconditionalSelectors = Object.keys(filtersBySelector);
+      unconditionalSelectorsDirty = false;
+    }
+    let selectors = specificOnly ? [] : unconditionalSelectors.slice();

[Sebastian Noack, 2016/05/25 07:30:49]

You can skip the unconditionalSelectorsDirty logic in the specificOnly case:

let selectors;
if (specificOnly)
{
  selectors = [];
}
else
{
  if (unconditionalSelectorsDirty)
  {
     unconditionalSelectors = Object.keys(filtersBySelector);
     unconditionalSelectorsDirty = false;
  }
  selectors = unconditionalSelectors.slice();
}

[Sebastian Noack, 2016/05/25 07:30:49]

Also note that if you'd call Object.keys() unconditionally, you can avoid the
isDirty logic and don't have to copy the list. But that would still be notably
slower? Just want to make sure, since the way you did it initally, the array was
copied anyway by Array.concat().

[Sebastian Noack, 2016/05/25 07:40:26]

One more thing, if we need that is dirty logic, how about just setting
unconditionalSelectors to null instead adding yet another global variable? That
way the code is a little shorter, and we won't leak outdated data.

[kzar, 2016/05/25 08:05:19]

Done.

[kzar, 2016/05/25 08:05:19]

I know I could skip restoring unconditionalSelectors if specificOnly is true but
there's not much point.

[kzar, 2016/05/25 08:05:19]

Yep, just double checked and it's still quite a bit slower using Object.keys()
every time even without the concat.

[Sebastian Noack, 2016/05/25 08:11:56]

Besides a speedup in specificOnly case which might be negligible, it also
improves the code locality and makes the purpose of that logic slightly easier
to understand IMO. Also it will be even shorter now where you only have one
variable.

[kzar, 2016/05/25 08:29:04]

I disagree on this one, but I see your point. I'll leave it up to Wladimir to
decide.

[Wladimir Palant, 2016/05/25 10:43:03]

Frankly, while I can see the arguments towards only setting
unconditionalSelectors when needed, it's not really important enough to be
changed. The flow won't necessarily become more readable if we do, and it's
already fine as is.

 
     let seenFilters = Object.create(null);
     let currentDomain = domain ? domain.toUpperCase() : "";
     while (true)
     {
       if (specificOnly && currentDomain == "")
         break;
 
       let filters = filtersByDomain[currentDomain];
       if (filters)
       {
-        for (let filterText in filters)
+        for (let filterKey in filters)
         {
-          if (filterText in seenFilters)
+          if (filterKey in seenFilters)
             continue;
-          seenFilters[filterText] = true;
+          seenFilters[filterKey] = true;
 
-          let filter = filters[filterText];
+          let filter = filters[filterKey];
           if (filter && !this.getException(filter, domain))
             selectors.push(filter.selector);
         }
       }
 
       if (currentDomain == "")
         break;
 
       let nextDot = currentDomain.indexOf(".");
       currentDomain = nextDot == -1 ? "" : currentDomain.substr(nextDot + 1);
     }
 
     return selectors;
   }
 };