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
Sorry about that.