Issue 4054 - Remove filters from filtersByDomain

lib/elemHide.js

Index: lib/elemHide.js
diff --git a/lib/elemHide.js b/lib/elemHide.js
index 6e2f1b56acbb6e67e79f779503b469f9d8829903..3d5bf2f5ea33972a7065d6622596443d999602da 100644
--- a/lib/elemHide.js
+++ b/lib/elemHide.js
@@ -202,6 +202,17 @@ var ElemHide = exports.ElemHide =
       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];

+      }
+    }
   },
 
   /**