Issue 5137 - [emscripten] Added basic filter storage implementation

Issue 5137 - [emscripten] Added basic filter storage implementation

[Wladimir Palant, 2017-05-01 14:47:25]

Note that this doesn't implement everything required in
https://issues.adblockplus.org/ticket/5137 - moving filters and managing hit
counts will be added separately.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
compiled/bindings/generator.cpp:329: }
I'm not really happy with the complexity required to expose namespaces. We
should probably discuss changes to the bindings definition API - maybe the code
can get considerably simpler if we go away from the embind-style APIs.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
compiled/bindings/generator.cpp:424: params.push_back(argName + " ? " + argName
+ "._pointer : 0");
Turns out we weren't supporting null for parameters expecting a class.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/m...
File compiled/bindings/main.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/m...
compiled/bindings/main.cpp:114: .function("isGeneric",
&UserDefinedSubscription::IsGeneric)
I need to add unit tests for this.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/m...
compiled/bindings/main.cpp:138: .function("getSubscriptionForFilter",
&FilterStorage::GetSubscriptionForFilter)
Note that the original name is getGroupForFilter - this has been renamed for
consistency. The original name is added as an alias in filterStorage.js.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/subscripti...
File compiled/subscription/UserDefinedSubscription.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/subscripti...
compiled/subscription/UserDefinedSubscription.cpp:36: FilterCategory::NONE,     
 // COMMENT
Turns out that our current code doesn't really support default subscriptions for
invalid and comment filters (which is probably a bug but not something worth
changing).

https://codereview.adblockplus.org/29426559/diff/29426560/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29426559/diff/29426560/lib/filterStorage.j...
lib/filterStorage.js:21: const {ArrayLike} = require("coreUtils");
This is left-over from a previous iteration, I already removed this locally.

https://codereview.adblockplus.org/29426559/diff/29426560/lib/subscriptionCla...
File lib/subscriptionClasses.js (right):

https://codereview.adblockplus.org/29426559/diff/29426560/lib/subscriptionCla...
lib/subscriptionClasses.js:22: const {ArrayLike} = require("coreUtils");
This is left-over from a previous iteration, I already removed this locally.

[Wladimir Palant, 2017-05-03 08:27:47]

Patch Set 2 removes the bogus ArrayLike imports. It also adjusts subscription
classes unit tests to changes in the way defaults are implemented - comments and
invalid filters no longer having "default" subscription targets.

[sergei, 2017-05-08 10:54:39]

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
compiled/bindings/generator.cpp:329: }
On 2017/05/01 14:47:25, Wladimir Palant wrote:
> I'm not really happy with the complexity required to expose namespaces. We
> should probably discuss changes to the bindings definition API - maybe the
code
> can get considerably simpler if we go away from the embind-style APIs.

I would like to include in that discussion the design, in particular global
variables like knownFilters, FilterNotifier and subscriptions (in
FilterStorage.cpp), not loosely coupling like Listed property of subscription,
and global things mentioned above like FilterNotifier and in some other places.
So far, I find these things OK but only because it closely repeats the original
JS version, so the number of bugs should be less, but later it would be better
to reconsider such things.

In particular here, FilterStorage should be a class and we will get rid of the
namespace. However I would like to have the whole library in a namespace, if
it's a bit tricky then we could use for instance ABP prefix for everything.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
compiled/bindings/generator.cpp:424: params.push_back(argName + " ? " + argName
+ "._pointer : 0");
On 2017/05/01 14:47:25, Wladimir Palant wrote:
> Turns out we weren't supporting null for parameters expecting a class.

In addition I wonder whether we can use C++ references, we do use them for
strings already, why not to use them also for other objects.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
File compiled/bindings/generator.h (right):

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
compiled/bindings/generator.h:183: typedef std::vector<PropertyInfo>
PropertyList;
It's usually a bad practice to call something like List, Set, Map, etc when it's
not important to emphasize the underlying data structure. Properties and Methods
would be better, though for me it's fine here.

https://codereview.adblockplus.org/29426559/diff/29428565/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29426559/diff/29428565/lib/filterStorage.j...
lib/filterStorage.js:72: subscription.delete();
strictly speaking it should be in finally block, but I'm not sure that we follow
that rule.

https://codereview.adblockplus.org/29426559/diff/29428565/lib/filterStorage.j...
lib/filterStorage.js:86: while (true)
Is this loop to remove duplicates of the filter in the same subscription? I
guess that a subscription is a set of unique filters, isn't it?

https://codereview.adblockplus.org/29426559/diff/29428565/test/filterStorage.js
File test/filterStorage.js (right):

https://codereview.adblockplus.org/29426559/diff/29428565/test/filterStorage....
test/filterStorage.js:68: changes.push(action + " " + subscription.url);
Is there any reason to not use arrays, changes.push([action,
subscription.url]);?

[Wladimir Palant, 2017-05-08 12:55:42]

Patch Set 3 has been rebased, so the interdiffs are slightly bogus. The only
real changes here are type names (PropertyList => Properties, MethodList =>
Methods) and an added finally block in JavaScript code.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
compiled/bindings/generator.cpp:329: }
On 2017/05/08 10:54:39, sergei wrote:
> In particular here, FilterStorage should be a class and we will get rid of the
> namespace.

I considered making FilterStorage a singleton class. The downside here is that
calling instance methods from JavaScript is considerably less efficient while
not offering any benefit here whatsoever.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
compiled/bindings/generator.cpp:424: params.push_back(argName + " ? " + argName
+ "._pointer : 0");
On 2017/05/08 10:54:39, sergei wrote:
> In addition I wonder whether we can use C++ references, we do use them for
> strings already, why not to use them also for other objects.

We could use references easily but these won't be references in JavaScript. The
bindings code will then have to make sure that no null pointers are passed in.
So far this didn't seem to be worth doing but we can certainly consider it.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
File compiled/bindings/generator.h (right):

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
compiled/bindings/generator.h:183: typedef std::vector<PropertyInfo>
PropertyList;
On 2017/05/08 10:54:39, sergei wrote:
> It's usually a bad practice to call something like List, Set, Map, etc when
it's
> not important to emphasize the underlying data structure. Properties and
Methods
> would be better, though for me it's fine here.

Done.

https://codereview.adblockplus.org/29426559/diff/29428565/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29426559/diff/29428565/lib/filterStorage.j...
lib/filterStorage.js:72: subscription.delete();
On 2017/05/08 10:54:39, sergei wrote:
> strictly speaking it should be in finally block, but I'm not sure that we
follow
> that rule.

You are correct. I tend to be too comfortable for unit test code, but for
production code we should get used to that pattern.

https://codereview.adblockplus.org/29426559/diff/29428565/lib/filterStorage.j...
lib/filterStorage.js:86: while (true)
On 2017/05/08 10:54:39, sergei wrote:
> Is this loop to remove duplicates of the filter in the same subscription? I
> guess that a subscription is a set of unique filters, isn't it?

No, a subscription can contain the same filter multiple times, we don't impose
any such restrictions (any more, since around Adblock Plus 0.7). This is
particularly important for comments, filter list authors tend to use comments as
separators - and these are identical then.

https://codereview.adblockplus.org/29426559/diff/29428565/test/filterStorage.js
File test/filterStorage.js (right):

https://codereview.adblockplus.org/29426559/diff/29428565/test/filterStorage....
test/filterStorage.js:68: changes.push(action + " " + subscription.url);
On 2017/05/08 10:54:39, sergei wrote:
> Is there any reason to not use arrays, changes.push([action,
> subscription.url]);?

This is code from the master branch adapted. But the reason is mostly: it's
easier to write tests, test.deepEqual(changes, ["subscription.added foo"]) is
easier to write and more readable than test.deepEqual(changes,
[["subscription.added", "foo"]]) - particularly once you get multiple changes.

[Wladimir Palant, 2017-05-10 12:38:22]

Patch Set 4 has been rebased on top of bindings generator changes already in the
repository and https://codereview.adblockplus.org/29434561/.

[Wladimir Palant, 2017-05-10 14:11:05]

In Patch Set 5 I've turned FilterStorage into a singleton class since this
appears to be the preferred solution. Nothing changed for the JS code, this is
handled entirely in the JS bindings. The bindings are easier to generate now,
but they also got slower - not a big issue currently since none of the methods
exposed by FilterStorage is a hot spot.

[Wladimir Palant, 2017-08-22 11:04:55]

Patch Set 6 has been rebased (copyright year update)

[hub, 2017-08-22 13:14:15]

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
File compiled/storage/FilterStorage.h (right):

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.h:34: unsigned int BINDINGS_EXPORTED
GetSubscriptionCount() const;
Line 30 you put the BINDINGS_EXPORTED before the return type. Here you put it
after.
Also in other source files, it is sometime before, sometime after.

Emscripten documentation show it after the return time (EMSCRIPTEN_KEEPALIVE
which is what BINDINGS_EXPORTED is #defined to)

[sergei, 2017-08-24 13:32:07]

My main concerns is only about changes lib/filterStorage.js, removing of the
parsing code does not look like a good idea. IMO it should be either restored or
be the very next thing to work on.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
compiled/bindings/generator.cpp:329: }
On 2017/05/08 12:55:41, Wladimir Palant wrote:
> On 2017/05/08 10:54:39, sergei wrote:
> > In particular here, FilterStorage should be a class and we will get rid of
the
> > namespace.
> 
> I considered making FilterStorage a singleton class. The downside here is that
> calling instance methods from JavaScript is considerably less efficient while
> not offering any benefit here whatsoever.

Singleton and global variables are actually equally bad from design point of
view. I would wait for finishing of the transformation, then take a look what
and how is actually used and then restructure in in way to get rid of these
global things without hurting the performance.

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
compiled/bindings/generator.cpp:424: params.push_back(argName + " ? " + argName
+ "._pointer : 0");
On 2017/05/08 12:55:41, Wladimir Palant wrote:
> On 2017/05/08 10:54:39, sergei wrote:
> > In addition I wonder whether we can use C++ references, we do use them for
> > strings already, why not to use them also for other objects.
> 
> We could use references easily but these won't be references in JavaScript.
The
> bindings code will then have to make sure that no null pointers are passed in.
> So far this didn't seem to be worth doing but we can certainly consider it.

Currently one has to manually test whether a pointer argument is null or not
each time in a method implementation, however many functions expect a real
object and this testing is just additional code which can be avoided. Therefore
using C++ references is safer and easier in many cases. However, if we
accidentally pass not a "real object" from JS into a function which does not
expect it, it would be better to generate the check into bindings than manually
implement it in a method implementation.

In addition I guess we should think about it a little bit more because it can
help to develop some other conventions, e.g. ref_counted objects should be
always allocated on heap and never on the stack.

https://codereview.adblockplus.org/29426559/diff/29435613/lib/filterStorage.js
File lib/filterStorage.js (right):

https://codereview.adblockplus.org/29426559/diff/29435613/lib/filterStorage.j...
lib/filterStorage.js:22: const {ActiveFilter} = require("filterClasses");
It seems this line is not required anymore.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
File compiled/storage/FilterStorage.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:43: for (unsigned int i = 0; i <
mSubscriptions.size(); i++)
Despite there are strangely no warnings from compiler and the number of
subscriptions will very unlikely exceed a dozen it would be better to not use
`int` and `unsigned int` so directly when it's actually
std::vector<>::size_type.
If it does not cause an overhead I would prefer to have `typedef unsigned int
size_type` as a member of FilterStorage, use it everywhere within FilterStorage
and not use `int`. Optionally -1 can also be defined in FilterStorage as an
invalid value.
And the same for Subscription classes.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:49: Subscription*
FilterStorage::GetSubscriptionForFilter(const Filter* filter)
It seems that this method should be const.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:90: bool
FilterStorage::RemoveSubscription(Subscription* subscription)
It seems so far there is no way to make the argument a constant pointer here and
in the adding method above.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:114: bool
FilterStorage::MoveSubscription(Subscription* subscription,
should `subscription` argument be a const pointer?

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:121: return false;
should there be an assert? it's seems undesired when `subscription` is not in
the list.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/subscripti...
File compiled/subscription/Subscription.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/subscripti...
compiled/subscription/Subscription.cpp:31: class DownloadableSubscription;
These declarations are unnecessary because the headers where these classes are
declared are already included.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/subscripti...
File compiled/subscription/Subscription.h (right):

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/subscripti...
compiled/subscription/Subscription.h:85: SUBSCRIPTION_PROPERTY(bool, mListed,
NONE, GetListed, SetListed);
To tell the truth I find this flag a bit questionable, but I think we can have
it.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/subscripti...
File compiled/subscription/UserDefinedSubscription.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/subscripti...
compiled/subscription/UserDefinedSubscription.cpp:97:
FilterNotifier::FilterChange(FilterNotifier::Topic::FILTER_REMOVED,
From the design point of view a subscription should not be aware about GetListed
and global FilterNotifier. But I think we can let it in for the sake of
progress.

https://codereview.adblockplus.org/29426559/diff/29523629/lib/filterStorage.js
File lib/filterStorage.js (left):

https://codereview.adblockplus.org/29426559/diff/29523629/lib/filterStorage.j...
lib/filterStorage.js:36: let formatVersion = 5;
I'm not sure that it's a good idea to already remove the parsing code.

[Wladimir Palant, 2017-08-31 11:32:37]

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
File compiled/bindings/generator.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29426560/compiled/bindings/g...
compiled/bindings/generator.cpp:424: params.push_back(argName + " ? " + argName
+ "._pointer : 0");
On 2017/08/24 13:32:04, sergei wrote:
> Currently one has to manually test whether a pointer argument is null or not
> each time in a method implementation, however many functions expect a real
> object and this testing is just additional code which can be avoided.
Therefore
> using C++ references is safer and easier in many cases. However, if we
> accidentally pass not a "real object" from JS into a function which does not
> expect it, it would be better to generate the check into bindings than
manually
> implement it in a method implementation.

I filed https://issues.adblockplus.org/ticket/5603 on that.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
File compiled/storage/FilterStorage.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:43: for (unsigned int i = 0; i <
mSubscriptions.size(); i++)
On 2017/08/24 13:32:06, sergei wrote:
> Despite there are strangely no warnings from compiler and the number of
> subscriptions will very unlikely exceed a dozen it would be better to not use
> `int` and `unsigned int` so directly when it's actually
> std::vector<>::size_type.
> If it does not cause an overhead I would prefer to have `typedef unsigned int
> size_type` as a member of FilterStorage, use it everywhere within
FilterStorage
> and not use `int`. Optionally -1 can also be defined in FilterStorage as an
> invalid value.
> And the same for Subscription classes.

Done.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:49: Subscription*
FilterStorage::GetSubscriptionForFilter(const Filter* filter)
On 2017/08/24 13:32:05, sergei wrote:
> It seems that this method should be const.

Managed to do this with somewhat counterintuitive changes.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:90: bool
FilterStorage::RemoveSubscription(Subscription* subscription)
On 2017/08/24 13:32:06, sergei wrote:
> It seems so far there is no way to make the argument a constant pointer here
and
> in the adding method above.

For AddSubscription() the argument shouldn't be constant - we store a non-const
pointer, and callers of SubscriptionAt() might modify the instance later. Here
the issue is smaller, but we are still calling SetListed(), changing reference
counter and notifying listeners with a non-const instance.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:114: bool
FilterStorage::MoveSubscription(Subscription* subscription,
On 2017/08/24 13:32:05, sergei wrote:
> should `subscription` argument be a const pointer?

It cannot be. Even if we make changes to the reference count possible on const
instances or manage to move the instance without touching the reference count,
the FilterNotifier call below requires a non-const instance. JavaScript
listeners might decide to modify the subscription.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:121: return false;
On 2017/08/24 13:32:05, sergei wrote:
> should there be an assert? it's seems undesired when `subscription` is not in
> the list.

Done.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
File compiled/storage/FilterStorage.h (right):

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.h:34: unsigned int BINDINGS_EXPORTED
GetSubscriptionCount() const;
On 2017/08/22 13:14:14, hub wrote:
> Line 30 you put the BINDINGS_EXPORTED before the return type. Here you put it
> after.

Done, it's consistent here now.

> Also in other source files, it is sometime before, sometime after.

Filed https://issues.adblockplus.org/ticket/5604 on this.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/subscripti...
File compiled/subscription/Subscription.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/subscripti...
compiled/subscription/Subscription.cpp:31: class DownloadableSubscription;
On 2017/08/24 13:32:06, sergei wrote:
> These declarations are unnecessary because the headers where these classes are
> declared are already included.

Done.

https://codereview.adblockplus.org/29426559/diff/29523629/lib/filterStorage.js
File lib/filterStorage.js (left):

https://codereview.adblockplus.org/29426559/diff/29523629/lib/filterStorage.j...
lib/filterStorage.js:36: let formatVersion = 5;
On 2017/08/24 13:32:07, sergei wrote:
> I'm not sure that it's a good idea to already remove the parsing code.

I can leave it in of course, but that would be merely a whole lot of dead code.
And I would have to mess with that dead code because the FilterStorage object is
now being defined elsewhere. Are you sure that you want it?

[sergei, 2017-08-31 12:43:40]

LGTM

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
File compiled/storage/FilterStorage.cpp (right):

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:49: Subscription*
FilterStorage::GetSubscriptionForFilter(const Filter* filter)
On 2017/08/31 11:32:36, Wladimir Palant wrote:
> On 2017/08/24 13:32:05, sergei wrote:
> > It seems that this method should be const.
> 
> Managed to do this with somewhat counterintuitive changes.

Yeah, sorry, maybe it was a bad idea to turn it into a const method,
`SubscriptionAt` should not be const either because it allows to modify
internals of constant FilterStorage.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:90: bool
FilterStorage::RemoveSubscription(Subscription* subscription)
On 2017/08/31 11:32:35, Wladimir Palant wrote:
> On 2017/08/24 13:32:06, sergei wrote:
> > It seems so far there is no way to make the argument a constant pointer here
> and
> > in the adding method above.
> 
> For AddSubscription() the argument shouldn't be constant - we store a
non-const
> pointer, and callers of SubscriptionAt() might modify the instance later. Here
> the issue is smaller, but we are still calling SetListed(), changing reference
> counter and notifying listeners with a non-const instance.

Acknowledged.

https://codereview.adblockplus.org/29426559/diff/29523629/compiled/storage/Fi...
compiled/storage/FilterStorage.cpp:114: bool
FilterStorage::MoveSubscription(Subscription* subscription,
On 2017/08/31 11:32:35, Wladimir Palant wrote:
> On 2017/08/24 13:32:05, sergei wrote:
> > should `subscription` argument be a const pointer?
> 
> It cannot be. Even if we make changes to the reference count possible on const
> instances or manage to move the instance without touching the reference count,
> the FilterNotifier call below requires a non-const instance. JavaScript
> listeners might decide to modify the subscription.

Acknowledged.