Opened on 03/23/2018 at 05:33:09 PM

Last modified on 05/23/2018 at 11:16:08 AM

#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.

Attachments (0)

Change History (8)

comment:1 Changed on 04/11/2018 at 02:50:52 PM by eric@adblockplus.org

  • Cc eric added

comment:2 Changed on 04/19/2018 at 11:12:45 AM by sergz

  • Cc greiner added

comment:3 Changed on 05/16/2018 at 02:38:14 PM by hfiguiere

  • Blocked By 6672 added

comment:4 Changed on 05/16/2018 at 02:42:49 PM by hfiguiere

  • Blocked By 6673 added

comment:5 Changed on 05/16/2018 at 02:47:07 PM by hfiguiere

  • Blocked By 6674 added

comment:6 Changed on 05/23/2018 at 10:57:59 AM by mjethani

  • Cc SabinaGreiner9 mjethani added; greiner removed

comment:7 Changed on 05/23/2018 at 11:07:14 AM by sergz

  • Cc greiner added; SabinaGreiner9 removed

comment:8 Changed on 05/23/2018 at 11:16:08 AM by mjethani

Sorry about that.

Add Comment

Modify Ticket

Change Properties
Action
as new .
as The resolution will be set. Next status will be 'closed'.
to The owner will be changed from (none).
Next status will be 'reviewing'.
 
Note: See TracTickets for help on using tickets.