Issue 4054 - Remove filters from filtersByDomain

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 139 matching lines @@
       let key;
       do {
         key = Math.random().toFixed(15).substr(5);
       } while (key in filterByKey);
 
       filterByKey[key] = filter;
       keyByFilter[filter.text] = key;
 
       if (usingFiltersByDomain)
       {
-        let domainMatches = filter.domains || defaultDomains;
-        for (let domain in domainMatches)
+        let domains = filter.domains || defaultDomains;
+        for (let domain in domains)
         {
           let filters = filtersByDomain[domain];
           if (!filters)
             filters = filtersByDomain[domain] = Object.create(null);
 
-          if (domainMatches[domain])
+          if (domains[domain])
             filters[filter.text] = filter;
           else
             filters[filter.text] = false;
         }
       }
 
       ElemHide.isDirty = true;
     }
   },
 
@@ skipping 16 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)
-    {
-      let domains = Object.keys(filter.domains || defaultDomains);

[Sebastian Noack, 2016/05/20 12:20:44]

So in the case here for..in would be actually slower? 

[kzar, 2016/05/20 12:49:29]

Well I didn't think about it either way to be honest, was just trying to get a
fix together quickly.

[kzar, 2016/05/20 13:13:38]

As we discussed in IRC I did a quick test of this in Chrome 50. Seems a bit
faster using for ... in, doesn't cause a deopt. Done.

-      for (let domain of domains)
-      {
-        let filters = filtersByDomain[domain];
-        if (filters)
-          delete filters[filter.text];

[kzar, 2016/05/20 11:36:12]

Note: There is a memory leak here. I don't delete filtersByDomain[domain] Object
even if the last filter just got removed for it. I couldn't figure out a way to
do that without performance suffering drastically. (As in Chrome thinking the
extension had crashed when disabling EasyList!)

Open to ideas, but if no one has any and the memory leak is a deal breaker I
guess we need to revert the earlier changes :/

[Wladimir Palant, 2016/05/20 12:17:51]

I don't think that this is a big deal. All filters being removed for a domain is
a rather unusual case - usually this only happens temporarily if at all.

[Sebastian Noack, 2016/05/20 12:20:44]

How did you try to delete filtersByDomain[domain]? The simplest way I can think
of would be like that:

if (filters)
{
  delete filters[filter.text];
  if (Object.keys(filters).length == 0)
    delete filtersByDomain[domain];
}

Perhaps, it gives some speed up to bail if there filter isn't in there:

if (filters && filter.text in filters)
{
  delete filters[filter.text];
  if (Object.keys(filters).length == 0)
    delete filtersByDomain[domain];
}

Another thing you could try would be using Object.getOwnPropertyNames() instead
Object.keys(). If it's still too slow, I guess I'd be fine with the memory leak,
after all we leak removed subscriptions anyway, IIRC.

[Sebastian Noack, 2016/05/20 12:29:24]

What if an element hiding filter is given in two subscriptions?

[Wladimir Palant, 2016/05/20 12:40:05]

Frankly, I don't care. No matter how you do it, it's not worth doing (looking at
both performance and code complexity).

[Wladimir Palant, 2016/05/20 12:40:05]

That case is being handled elsewhere (FilterListener). If remove() is called
here, then the filter actually needs to be removed. Note also the `if
(!(filter.text in keyByFilter))` check about.

However, I just noticed that this block is misplaced - it should be inside the
`else` block above (meaning: should not apply to exceptions). So NOT LGTM yet
after all.

[kzar, 2016/05/20 12:49:29]

Whoops sorry, I moved this at the last minute and apparently messed it up...
Done.

[kzar, 2016/05/20 12:49:29]

I didn't consider that. I'm not sure how we can handle that to be honest.

[kzar, 2016/05/20 12:49:29]

Well I tried those ideas already except using .getOwnPropertyNames() but I don't
think that would make much difference. I also tried something like this but it
was still hugely (extension crashes) slow:

if (Object.keys(filters).length == 1)
  delete filtersByDomain[domain];
else
  delete filters[filter.text];

+
+      if (usingFiltersByDomain)
+      {
+        let domains = filter.domains || defaultDomains;
+        for (let domain in domains)
+        {
+          let filters = filtersByDomain[domain];
+          if (filters)
+            delete filters[filter.text];
+        }
       }
     }
   },
 
   /**
    * Checks whether an exception rule is registered for a filter on a particular
    * domain.
    */
   getException: function(/**Filter*/ filter, /**String*/ docDomain) /**ElemHideException*/
   {
@@ skipping 227 matching lines @@
       if (currentDomain == "")
         break;
 
       let nextDot = currentDomain.indexOf(".");
       currentDomain = nextDot == -1 ? "" : currentDomain.substr(nextDot + 1);
     }
 
     return selectors;
   }
 };