Move filter state to a global map

[mjethani]

Background

After #7094 and #7029, we would now like to make element hiding filter objects ephemeral (#7452). But we still cannot do this because some objects may contain state, like whether the filter is disabled, how many hits it got, and so on. We previously discussed the idea of removing the state properties (#7095) but some clients, including possibly the WebExt client in the future, need these properties or some other way to access the state of a filter.

This is a proposal for an alternative approach.

We could move the state of a filter into a global map. As an example, let's take the current implementation of the disabled property:

  get disabled()
  {
    return this._disabled;
  }

  set disabled(value)
  {
    let oldValue = this._disabled;

    if (value != oldValue)
    {
      this._disabled = value;
      filterNotifier.emit("filter.disabled", this, value, oldValue);
    }
  }

The new implementation might look something like this instead:

  get disabled()
  {
    if (typeof this._disabled == "undefined")
      this._disabled = filterState.get(this._text) || false;

    return this._disabled;
  }

  set disabled(value)
  {
    let oldValue = this.disabled;

    if (value != oldValue)
    {
      filterState.set(this._text, this._disabled = !!value);
      filterNotifier.emit("filter.disabled", this, value, oldValue);
    }
  }

Only the first get in a given temporary object would entail a lookup in a global map; for subsequent gets, the value would be readily available. In the case of set, only if the value is set to a different value than the current value, it would entail a write to a global map.

Reads and writes from and to Map objects, though nowhere as fast as direct property access, are still quite fast in practice. Note that there is already a lookup in the setter for emitting the event.

If we do this, we will be able to free up most of the filter objects in memory while still keeping the state for the few that do have some state.

What to change

TBD

[greiner]

Note that we already access filter properties (so far only disabled and reason) for all filters in custom subscriptions to be able to display them accordingly in the upcoming UI and that we may expand it to filters from downloadable subscriptions in the future. It'd be great if that remains possible without destroying all the memory benefits this approach gives us due to the setting of the filter's _disabled property that occurs in the getter.

[mjethani]

I have been working on a new API for filter state.

It works like this:

let {Filter} = require("./lib/filterClasses");
let {filterState} = require("./lib/filterState");
let {filterNotifier} = require("./lib/filterNotifier");

filterNotifier.on(
  "filter.disabled",
  (text, value, oldValue) => console.log("filter.disabled", text,
                                         value, oldValue)
);

filterNotifier.on(
  "filter.hitCount",
  (text, value, oldValue) => console.log("filter.hitCount", text,
                                         value, oldValue)
);

filterNotifier.on(
  "filter.lastHit",
  (text, value, oldValue) => console.log("filter.lastHit", text,
                                         value, oldValue)
);

let filter = Filter.fromText("foo");

let {text} = filter;
let state = filterState.get(text);

// Prints { disabled: false, hitCount: 0, lastHit: 0 }
console.log(state);

state.disabled = true;

// Prints { disabled: false, hitCount: 0, lastHit: 0 }
console.log(filterState.get(text));

filterState.set(text, state);

// Prints { disabled: true, hitCount: 0, lastHit: 0 }
console.log(filterState.get(text));

state.unknownProperty = 123;
filterState.set(text, state);

// Prints { disabled: true, hitCount: 0, lastHit: 0, unknownProperty: 123 }
console.log(filterState.get(text));

delete state.disabled;
filterState.set(text, state);

// Prints { disabled: false, hitCount: 0, lastHit: 0, unknownProperty: 123 }
console.log(filterState.get(text));

filterState.set(text, {hitCount: 1000, lastHit: 1558275974647});

// Prints { disabled: false, hitCount: 1000, lastHit: 1558275974647 }
console.log(filterState.get(text));

Output:

{ disabled: false, hitCount: 0, lastHit: 0 }
{ disabled: false, hitCount: 0, lastHit: 0 }
filter.disabled foo true false
{ disabled: true, hitCount: 0, lastHit: 0 }
{ disabled: true, hitCount: 0, lastHit: 0, unknownProperty: 123 }
filter.disabled foo false true
{ disabled: false, hitCount: 0, lastHit: 0, unknownProperty: 123 }
filter.hitCount foo 1000 0
filter.lastHit foo 1558275974647 0
{ disabled: false, hitCount: 1000, lastHit: 1558275974647 }

[greiner]

Mind elaborating on the benefits of this proposal? Because I can mostly see drawbacks because it encourages filter states being duplicated and getting out-of-sync.

Why not just interact with the filter state directly (e.g. using Proxy since Object.observe() doesn't exist anymore) rather than manipulating some mutable placeholder state object? That way nothing can get out-of-sync. We can still batch modify the state, if that's one of the issues you're trying to solve.

For example:

let filter = Filter.fromText("foo");
let {text} = filter;
let state = filterState.get(text);

// update single property
state.disabled = true; // state.disabled === true
delete state.disabled; // state.disabled === false

// batch modify
state.set({hitCount: 1000, lastHit: 1558275974647);

Alternatively, if there's no good way to implement this efficiently, we could fall back to a bare-bones, REST-like API such as:

let filter = Filter.fromText("foo");
let {text} = filter;

filterState.delete(text, "disabled");
filterState.set(text, "disabled", true);
filterState.set(text, {hitCount: 1000, lastHit: 1558275974647});

It may not be pretty but it does the job without introducing more complexity.

[sebastian]

Sorry, but we switched to GitLab. If this issue is still relevant, please file it again in the ​new issue tracker.