Opened 5 months ago

Closed 4 months ago

Last modified 3 weeks ago

#6727 closed change (fixed)

Use string rather than map for single-domain filters

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

https://codereview.adblockplus.org/29791555/

Description (last modified by mjethani)

Background

Adblock Plus's memory footprint is an important concern. One of the areas where this issue can be tackled is the domains property of the ActiveFilter class. The Map instance for ActiveFilter.domains is quite heavy; even with only 3 entries, it can have a shallow size of ~150 bytes, which for ~65,000 filters is ~10 MB on the heap. We can reduce the memory usage here by replacing all Map instances for single-domain filters (e.g. example.com##.foo) with just the domain name. In my tests, this saves ~3 MB on the heap with EasyList+AA.

Out of ~30,000 element hiding and emulation filters and exceptions in EasyList+AA, ~12,000 are single-domain cases.

What to change

In the domains getter of the ActiveFilter class prototype, for the single-domain case, set the value of the property to just the name of the domain. Update the domains getter to return a temporary Map instance for the single-domain case.

Hints for testers

Make sure that domain matching still works correctly, for both blocking and hiding filters, with domain exclusions (example.com,~www.example.com##.foo) and exceptions (example.com#@#.foo), for no-domain, single-domain, and multi-domain cases.

For example, example.com,~www.example.com##.foo should apply on https://example.com/ but not on https://www.example.com/ or if there are two filters ##.foo and www.example.com#@#.foo then the first one should apply on https://example.com/ but not on https://www.example.com/

Change History (18)

comment:1 Changed 5 months ago by mjethani

@kzar Sergei seems to agree that this is a good idea, what do you think?

comment:2 Changed 5 months ago by mjethani

  • Review URL(s) modified (diff)

comment:3 Changed 5 months ago by mjethani

  • Owner set to mjethani

comment:4 Changed 5 months ago by kzar

  • Priority changed from Unknown to P2
  • Ready set

Sounds like a good idea to me, if we can save even 2 MB for our users then we should do it.

comment:5 Changed 4 months ago by abpbot

A commit referencing this issue has landed:
Issue 6727 - Use string rather than map for single-domain filters

comment:6 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:7 Changed 4 months ago by mjethani

  • Resolution set to fixed
  • Status changed from new to closed

comment:8 Changed 4 months ago by mjethani

With changes like these, we have brought the memory footprint down by ~9.2 MB since changeset e59042524857.

comment:9 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:10 Changed 3 months ago by kzar

Please could you briefly explain what should happen for those examples? I just mean to give a couple of examples of domains which a filter would/wouldn't apply to with the examples. That would make testing much easier I think.

comment:11 Changed 3 months ago by mjethani

@kzar don't the testers already know what should happen? This is not a new feature, we have only modified an implementation detail that should have no visible effect on the outcome.

comment:12 Changed 3 months ago by kzar

Well, the idea is to make life as easy as possible for the testers. It'll speed up the QA process, and makes regressions slipping through less likely. It's good you gave some example filters, they very quickly illustrate roughly what needs to be tested. If you give some quick examples of what the desired outcomes (blocked/not blocked domains) are expected to be it'll help even more.

comment:13 Changed 3 months ago by mjethani

But aren't these desired outcomes already covered by the current tests? I would say just run all the tests again to make sure everything is in order. There is no new functionality, the old functionality is already covered by the existing tests, so I'm not sure what I should add here.

comment:14 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:15 Changed 3 months ago by mjethani

Alright, I have added some concrete examples now.

comment:16 Changed 3 months ago by kzar

Thanks, I appreciate it.

comment:17 Changed 2 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Domain matching still appears to be working correctly for both hiding and blocking filters (plus exceptions)

ABP 3.2.0.2103
Chrome 68 / 55 / 49 / Windows 10
Firefox 61 / 55 / 51 / Windows 10

comment:18 Changed 3 weeks ago by mjethani

  • Blocking 7000 added
Note: See TracTickets for help on using tickets.