Opened 12 months ago

Closed 11 months ago

Last modified 11 months ago

#7095 closed change (rejected)

Remove unused filter state properties

Reported by: mjethani Assignee:
Priority: P2 Milestone:
Module: Core Keywords:
Cc: greiner, sebastian, kzar, hfiguiere, sergz Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by 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.

Change History (17)

comment:1 Changed 12 months ago by mjethani

  • Description modified (diff)

comment:2 Changed 12 months ago by mjethani

  • Summary changed from Remove unused filter client-side state properties to Remove unused filter state properties

comment:3 Changed 12 months ago by mjethani

@greiner @sebastian any thoughts?

comment:4 Changed 12 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 12 months ago by 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.

comment:6 Changed 12 months ago by mjethani

  • Cc sergz added

comment:7 Changed 12 months ago by mjethani

  • Blocking 7097 added

comment:8 Changed 12 months ago by mjethani

  • Blocking 7094 removed

comment:9 Changed 12 months ago by 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.

Last edited 12 months ago by mjethani (previous) (diff)

comment:10 Changed 12 months ago by 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.

comment:11 Changed 12 months ago by 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?

comment:12 Changed 12 months ago by 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.

comment:13 Changed 12 months ago by 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.

comment:14 Changed 11 months ago by mjethani

  • Blocking 7097 removed

comment:15 Changed 11 months ago by mjethani

  • Description modified (diff)
  • Resolution set to rejected
  • Status changed from new to closed

comment:16 Changed 11 months ago by kzar

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

comment:17 Changed 11 months ago by 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).

Note: See TracTickets for help on using tickets.