Opened 8 months ago

Closed 8 months ago

Last modified 4 months ago

#7324 closed change (fixed)

Simplify logic for domains property

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

https://codereview.adblockplus.org/30022555/
https://gitlab.com/eyeo/adblockplus/adblockpluscore/merge_requests/25

Description (last modified by mjethani)

Background

See patch #1 and patch #2.

With the changes for #6815, #7046, and #7265, the implementation of the domains property getter in the ActiveFilter class has become somewhat complex. Now it can be simplified in the following manner:

  1. Always cache the Map object for the domains property using a Cache object (#7285) with some reasonable capacity (1,000 works)
  2. Keep the Map object in the ActiveFilter object itself only if domainSource is non-null

In my tests with the above patches, I got the following results:

  1. Loading of the subscriptions speeds up slightly
  2. Peak memory usage (heap allocated) on startup goes from ~51.5 MB to ~45 MB
  3. Initial memory usage (heap allocated) after initial GC goes from ~39 MB to ~36.2 MB
  4. The performance of request blocking and element hiding (style sheet generation) is not significantly affected

This change also simplifies the implementation logic and the tests.

What to change

As described above.

Hints for testers

Follow the hints for #6815 and #7265.

Change History (15)

comment:1 Changed 8 months ago by mjethani

  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set
  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 Changed 8 months ago by mjethani

  • Description modified (diff)
  • Owner set to mjethani

comment:3 Changed 8 months ago by mjethani

  • Blocking 7000 added

comment:4 Changed 8 months ago by mjethani

  • Description modified (diff)

comment:5 Changed 8 months ago by abpbot

A commit referencing this issue has landed:
Issue 7324 - Simplify logic for domains property

comment:6 Changed 8 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:7 Changed 8 months ago by mjethani

  • Cc hfiguiere added
  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:8 Changed 8 months ago by mjethani

I have reopened this because the change here has in fact caused a serious performance regression in element hiding. I had tested with EasyList only. When I tested with EasyList+AA, the code spent a significant amount of time in the domains getter, called from getException in lib/elemHideExceptions.js.

Now we have these options:

  1. Revert the change: I do not want to do this, because the idea was to simplify the logic and make it less hacky
  2. Revert part of the change by bringing back the _cacheDomains property: I don't like this because I am trying to move away from the expectation that filter objects will be persistent in memory
  3. Organize exceptions by domain
  4. Some other way of avoiding repeated calls to the domains getter

I'll try to work this out and leave this ticket open until then. In the worst case we go with option 2 above.

comment:9 Changed 8 months ago by mjethani

  • Blocking 7355 added; 7000 removed

comment:10 Changed 8 months ago by mjethani

  • Review URL(s) modified (diff)

comment:11 Changed 8 months ago by mjethani

  • Status changed from reopened to reviewing

comment:12 Changed 8 months ago by abpbot

A commit referencing this issue has landed:
Issue 7324 - Set _domains property for non-empty domain maps

comment:13 Changed 8 months ago by mjethani

  • Description modified (diff)

comment:14 Changed 8 months ago by mjethani

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

comment:15 Changed 4 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Domain matching still works as expected.

ABP 0.9.15.2340
Microsoft Edge 44.17763.1.0 / Windows 10 1809

ABP 3.5.2.2340
Chrome 49.0.2623.75 / Windows 10 1809
Chrome 75.0.3770.142 / Windows 10 1809
Opera 36.0.2130.65 / Windows 10 1809
Opera 62.0.3331.72 / Windows 10 1809
Firefox 51.0 / Windows 10 1809
Firefox 68.0 / Windows 10 1809
Firefox Mobile 68.0 / Android 7.2.2

Note: See TracTickets for help on using tickets.