Opened 10 months ago

Last modified 8 months ago

#6520 new change

Use filter classes natively from emscripten branch of adblockpluscore

Reported by: sergz Assignee:
Priority: P2 Milestone:
Module: Libadblockplus Keywords:
Cc: hfiguiere, rjeschke, oleksandr, eric, greiner, mjethani Blocked By: #6452, #6672, #6673, #6674
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

Background

It's the next step after #6452. We put a hope that it will help us to address several points which are very sensitive for us right now. Additionally doing it right now can help to identify, or at least reject, the way to deal with objects from emscripten world (for more details see #hub:7477).

The proposal

please let me know if you think that the proposal is worth moving to a separate issue.

background
  • There is Filter.knownFilters, which is never shrinking, namely there is no code removing already added entries. Despite it's a very questionable design decision it can be useful here because by fact one may not care about deletion of filter objects so far.
  • We could start with other classes, in particular with matcher, elemhide, subscriptions, etc, but since it's a lot of new code, and requires a lot of time, it seems safer to firstly simply refactor (without changing the functionality) and then replace only filter classes. It also targets the part demonstrating the most memory consumption.
changes

I would like to propose to not use filter classes objects in core JS code and switch to C-like api, namely a filter object should be considered as an opaque pointer, and for example in order to get a filter text or domains one should rather call Filter.getText(filter), Filter.getDomains(filter), etc. In the pure JS implementation these methods will just forward the calls to the corresponding methods of passed filter object, however in C++ code there are options. Without plunging into details, it would allow us to have native C++ filter classes without instances of filter classes in JS VM world.

Now details and Concerns

  • One should measure the impact on the performance, though I expect it to be negligible.
  • We could store all filters as a member of FilterEngine what would be an analogue of Filter.knownFilters.
  • perhaps we can even experiment with internal representation of all filters (e.g., as discussed use trie, and please check out what some other adblockers are doing).
  • I deliberately added Filter.getText(filter) because although filter textual representation looks very attractive (it already uniquely identifies a filter, easy to debug, does not require a deleting method because it's JS primitive, etc) it can be not the most efficient candidate. With the filter text we will still have to create a huge number of strings in JS VM, additionally it means that there will be additional look up delay dependent on the filter text for each call of Filter.what-ever-api. If we know that filters are never deleted then we could use the integer representation of the pointer to an instance of a filter, alternatively even if we proceed with a textual representation there can be some optimizations and it's not necessarily to be a filter text.
  • it seems that having such C-like API and native filter classes allows us independently try different things, perhaps even e.g. new matcher from emscripten, either in current JS core or only in C++.

PS

Please feel free to accomplish the issue description, discuss in the comments and file the following up issues.

Change History (8)

comment:1 Changed 10 months ago by eric@…

  • Cc eric added

comment:2 Changed 9 months ago by sergz

  • Cc greiner added

comment:3 Changed 8 months ago by hfiguiere

  • Blocked By 6672 added

comment:4 Changed 8 months ago by hfiguiere

  • Blocked By 6673 added

comment:5 Changed 8 months ago by hfiguiere

  • Blocked By 6674 added

comment:6 Changed 8 months ago by mjethani

  • Cc SabinaGreiner9 mjethani added; greiner removed

comment:7 Changed 8 months ago by sergz

  • Cc greiner added; SabinaGreiner9 removed

comment:8 Changed 8 months ago by mjethani

Sorry about that.

Note: See TracTickets for help on using tickets.