Remove unused filter state properties

[mjethani]

Background

There are currently three properties of the Filter class that keep client-side state for a filter: disabled, hitCount, and lastHit. These properties are no longer being used in the browser extension for Chrome, Firefox, and Opera. Because of these properties, it is not entirely possible to lose references to Filter objects (#7094) (because the state would be lost).

If these properties are not being used on any product based on the core library, we should remove them. It would allow us to make some more optimizations.

Resolution

Based on the discussion in this ticket, we have decided not to remove these properties as they may be needed in the future. Instead, objects with some state will be persisted in memory. It should still be possible to do the optimization in #7097 for most filter objects.

[mjethani]

@greiner @sebastian any thoughts?

[sebastian]

If we can get rid of those properties, the memory and disk usage improvements will probably be significant, as most boilerplate in the serialized filter data come from persisting these properties.

[mjethani]

The plan is to make filter objects stateless and then ephemeral (#7097), at least the most lightweight of them. In the case of ElemHideFilter instances, we don't need them at all, because we are storing all the same information in lib/elemHide.js. For RegExpFilter instances with a literal string pattern (#7052), we don't need those objects either.

Regarding the statefulness of these objects, I already have a solution for Filter.prototype.subscriptions (#7096).

Now I'd like to know if anyone has any objections about removing the above three properties. They are not being used in the Chrome and Firefox extension, from what I can tell.

[greiner]

We're going to need the "disabled" property for the UI to show whether a given filter is disabled and to allow users to en-/disable it. This is part of the ​new custom filters editor table that will be included in the next release.

Also note that en-/disabling the extension on a page may cause filters in downloadable filter lists to get disabled.

The other two properties aren't being populated at this point so removing those shouldn't affect us AFAIK but they would be interesting for use in that same table as well as for the ​opt-in data collection we're planning to add.

[mjethani]

Are only user-defined filters supposed to be disabled? If it's for downloadable filters too, then is it per subscription or across all subscriptions?

[greiner]

For the custom filters table it should be sufficient if only the filter within the custom subscription gets disabled.

For reenabling the extension, however, a filter is expected to be disabled across all subscriptions (see ​adblockpluschrome/lib/whitelisting.js), regardless of whether or not it was added by the user.

[mjethani]

Oh, I see.

For disabled filters, we could just maintain a separate list and put them there. At the time of serialization, we could write them out as normal. I'll see if this is feasible and makes sense.

So instead of all filter objects being stateless, the code would have to distinguish between the objects that have any state and then just leave them alone instead of removing them from memory. This is fine. We could still benefit from this optimization a lot for most filters.

[kzar]

I wonder if we could create a WeakMap of disabled filters, then use that from a disabled getter in the Filter instances?

[mjethani]

Yeah, something like that.

It depends on which other parts of the code are holding on to the Filter instances. If there are no other references to the object, then it would just have to be a Map (otherwise even this last reference would be lost). Essentially, I think that Filter.knownFilters could hold on to any Filter instances that have any state at all, but leave all the other Filter instances out - they can be created on demand (transparently to the caller). In most cases we don't need Filter objects at all, just the text (#7097).