Issue 29886685: Issue 6856 - Remove FilterStorage.moveSubscription (Closed)
|
Created:
Sept. 20, 2018, 7:33 p.m. by Jon Sonesen Modified:
Oct. 2, 2018, 5:16 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore Visibility:
Public. |
DescriptionIssue 6856 - Remove FilterStorage.moveSubscription
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address PS1 Comments #
Total comments: 1
Patch Set 3 : Remove erroneous subscription.moved listener #Patch Set 4 : Fix JSdoc String #
Total comments: 9
Patch Set 5 : Address PS4 Comment #
Total comments: 9
Patch Set 6 : Address PS5 Comments #Patch Set 7 : Address PS6 Comment #Patch Set 8 : Address PS7 comments #
MessagesTotal messages: 18
Patch Set 1 Some initial comment, I'll get into this in more detail in the evening. https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.j... lib/filterStorage.js:83: * Generator for filter subscriptions containing all filters We could just say "Yields filter subscriptions containing all filters." https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.j... lib/filterStorage.js:84: * @type {Subscription.Generator} This is a function now rather than a property, so we don't use `@type` but rather `@returns`, or in this case `@yields` (since it's a generator function). @yields {Subscription} https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.j... lib/filterStorage.js:88: for (let subscription of this.knownSubscriptions.values()) We could replace the body of this function with this: yield* this.knownSubscriptions.values(); `yield*` is essentially syntactic sugar, it does what a `for` loop does above but with one line of code. We use `yield*` in other places so compatibility is not an issue. https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.j... lib/filterStorage.js:92: subscriptionCount() This needs JSDoc. How about: Number of known subscriptions. @type {number} Also this should be a property rather than a function, so we should replace `subscriptionCount()` with `get subscriptionCount()` here. https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.j... lib/filterStorage.js:153: return; It seems when we have a check like this we normally leave a blank line after it.
Thanks Manish, sorry for the delay. Gonna try and knock this out asap. https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.j... lib/filterStorage.js:83: * Generator for filter subscriptions containing all filters On 2018/09/21 09:21:58, Manish Jethani wrote: > We could just say "Yields filter subscriptions containing all filters." Done. https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.j... lib/filterStorage.js:84: * @type {Subscription.Generator} On 2018/09/21 09:21:58, Manish Jethani wrote: > This is a function now rather than a property, so we don't use `@type` but > rather `@returns`, or in this case `@yields` (since it's a generator function). > > @yields {Subscription} Done. https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.j... lib/filterStorage.js:88: for (let subscription of this.knownSubscriptions.values()) On 2018/09/21 09:21:58, Manish Jethani wrote: > We could replace the body of this function with this: > > yield* this.knownSubscriptions.values(); > > `yield*` is essentially syntactic sugar, it does what a `for` loop does above > but with one line of code. We use `yield*` in other places so compatibility is > not an issue. Done. https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.j... lib/filterStorage.js:92: subscriptionCount() On 2018/09/21 09:21:58, Manish Jethani wrote: > This needs JSDoc. > > How about: > > Number of known subscriptions. > @type {number} > > Also this should be a property rather than a function, so we should replace > `subscriptionCount()` with `get subscriptionCount()` here. Done. https://codereview.adblockplus.org/29886685/diff/29886686/lib/filterStorage.j... lib/filterStorage.js:153: return; On 2018/09/21 09:21:59, Manish Jethani wrote: > It seems when we have a check like this we normally leave a blank line after it. Done. https://codereview.adblockplus.org/29886685/diff/29893567/lib/filterListener.js File lib/filterListener.js (right): https://codereview.adblockplus.org/29886685/diff/29893567/lib/filterListener.... lib/filterListener.js:154: subscription.url == "https://easylist-downloads.adblockplus.org/abp-filters-anti-cv.txt" || Pretty sure this is just a rebase change
I'll take another look at this tomorrow, but a couple of comments for now. https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.j... lib/filterStorage.js:93: * @returns {number} This should be `@type {number}` since it's a property, not a method. https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.j... lib/filterStorage.js:447: let subscriptions = [...this.subscriptions()].filter( I think we should remove this and just add the following to the loops below: if (subscription instanceof ExternalSubscription) continue;
I'll wait on the rest of the comments before adding a new patch then. Thanks
https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.j... lib/filterStorage.js:447: let subscriptions = [...this.subscriptions()].filter( On 2018/09/27 17:42:54, Manish Jethani wrote: > I think we should remove this and just add the following to the loops below: > > if (subscription instanceof ExternalSubscription) > continue; Can the iterations be consolidated as well?
https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.j... lib/filterStorage.js:447: let subscriptions = [...this.subscriptions()].filter( On 2018/09/27 18:46:59, Jon Sonesen wrote: > On 2018/09/27 17:42:54, Manish Jethani wrote: > > I think we should remove this and just add the following to the loops below: > > > > if (subscription instanceof ExternalSubscription) > > continue; > > Can the iterations be consolidated as well? It doesn't look like, because all the subscriptions need to be serialized first before each subscription's filters. We would already be avoiding two loops of sorts by skipping that code, but then we'd have to do an additional `instanceof` check for each subscription. How about this instead: let exportableSubscriptions = []; for (let subscription of this._subscriptions()) { if (!(subscription instanceof ExternalSubscription)) exportableSubscriptions.push(subscription); } So basically I'm saying we inline the call to the filter method and skip the creation of an additional temporary array.
I still want to take a proper look at the whole patch as well as related code, but I have one comment I wanted to get across right now. https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.j... lib/filterStorage.js:447: let subscriptions = [...this.subscriptions()].filter( On 2018/09/28 08:15:49, Manish Jethani wrote: > On 2018/09/27 18:46:59, Jon Sonesen wrote: > > On 2018/09/27 17:42:54, Manish Jethani wrote: > > > I think we should remove this and just add the following to the loops below: > > > > > > if (subscription instanceof ExternalSubscription) > > > continue; > > > > Can the iterations be consolidated as well? > > It doesn't look like, because all the subscriptions need to be serialized first > before each subscription's filters. > > [...] OK, scratch this. Let's leave it as it is (in the current patch). The reason I'm saying is that I now realize that global state, including the contents of the knownSubscriptions map, can change during the execution of this method. Remember this is a generator, which means it exits at each yield statement, then comes back to where it left (possibly after a million years). This means, technically, we should make a copy of the _current_ subscriptions to export. I would say push to a temporary array using a `for` loop, but it seems the spread operator is very efficient. https://jsperf.com/set-iterator-vs-foreach/4 Anyway, this is a hotspot, so we can revisit this function when we're optimizing. This looks OK to me for now.
https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.j... lib/filterStorage.js:93: * @returns {number} On 2018/09/27 17:42:54, Manish Jethani wrote: > This should be `@type {number}` since it's a property, not a method. Done. https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.j... lib/filterStorage.js:447: let subscriptions = [...this.subscriptions()].filter( On 2018/09/28 22:33:25, Manish Jethani wrote: > On 2018/09/28 08:15:49, Manish Jethani wrote: > > On 2018/09/27 18:46:59, Jon Sonesen wrote: > > > On 2018/09/27 17:42:54, Manish Jethani wrote: > > > > I think we should remove this and just add the following to the loops > below: > > > > > > > > if (subscription instanceof ExternalSubscription) > > > > continue; > > > > > > Can the iterations be consolidated as well? > > > > It doesn't look like, because all the subscriptions need to be serialized > first > > before each subscription's filters. > > > > [...] > > OK, scratch this. Let's leave it as it is (in the current patch). > > The reason I'm saying is that I now realize that global state, including the > contents of the knownSubscriptions map, can change during the execution of this > method. Remember this is a generator, which means it exits at each yield > statement, then comes back to where it left (possibly after a million years). > This means, technically, we should make a copy of the _current_ subscriptions to > export. > > I would say push to a temporary array using a `for` loop, but it seems the > spread operator is very efficient. > > https://jsperf.com/set-iterator-vs-foreach/4 > > Anyway, this is a hotspot, so we can revisit this function when we're > optimizing. This looks OK to me for now. Acknowledged.
https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.j... lib/filterStorage.js:115: for (let subscription of FilterStorage.subscriptions()) Since this is within the FilterStorage object, maybe it's better to directly access the knownSubscriptions object here? (It is private to the object, but code within the object should be able to access it directly) So like `FilterStorage.knownSubscriptions.values()` instead of `FilterStorage.subscriptions()` is what I was thinking. https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.j... lib/filterStorage.js:373: if (this.subscriptionCount == 0) Here (and below) too I think it's probably better to just access `this.knownSubscriptions.size` directly.
https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.j... lib/filterStorage.js:376: this.subscriptions = parser.subscriptions; Note: An implication of this change is that a patterns.ini (the filter that contains the subscriptions serialized to disk) with duplicate subscriptions will be read as having only one subscription, the first occurrence. I have to think about the implications of this, but I'll make a note in the Trac ticket.
https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.j... lib/filterStorage.js:376: this.subscriptions = parser.subscriptions; On 2018/10/01 10:53:13, Manish Jethani wrote: > Note: An implication of this change is that a patterns.ini (the filter that > contains the subscriptions serialized to disk) with duplicate subscriptions will > be read as having only one subscription, the first occurrence. I have to think > about the implications of this, but I'll make a note in the Trac ticket. Acknowledged. https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.j... lib/filterStorage.js:115: for (let subscription of FilterStorage.subscriptions()) On 2018/10/01 10:40:45, Manish Jethani wrote: > Since this is within the FilterStorage object, maybe it's better to directly > access the knownSubscriptions object here? (It is private to the object, but > code within the object should be able to access it directly) > > So like `FilterStorage.knownSubscriptions.values()` instead of > `FilterStorage.subscriptions()` is what I was thinking. Yeah, I think you are right. I had the same thought while writing this but figured to just stick with the previous convention to see what happened in review. Also, convention was my only argument for not changing. Which kinda makes less sense now. Although (and I hadn't thought of this previously) are we hurting ourselves performance wise? As far as I have read lists are much faster for access and iteration, is this wrong? https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.j... lib/filterStorage.js:373: if (this.subscriptionCount == 0) On 2018/10/01 10:40:45, Manish Jethani wrote: > Here (and below) too I think it's probably better to just access > `this.knownSubscriptions.size` directly. Acknowledged.
https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.j... lib/filterStorage.js:115: for (let subscription of FilterStorage.subscriptions()) On 2018/10/01 15:40:57, Jon Sonesen wrote: > On 2018/10/01 10:40:45, Manish Jethani wrote: > > Since this is within the FilterStorage object, maybe it's better to directly > > access the knownSubscriptions object here? (It is private to the object, but > > code within the object should be able to access it directly) > > > > So like `FilterStorage.knownSubscriptions.values()` instead of > > `FilterStorage.subscriptions()` is what I was thinking. > > Yeah, I think you are right. I had the same thought while writing this but > figured to just stick with the previous convention to see what happened in > review. Also, convention was my only argument for not changing. Which kinda > makes less sense now. Although (and I hadn't thought of this previously) are we > hurting ourselves performance wise? I'm not sure, but it shouldn't hurt if we just avoid an additional function call either, unless doing so makes the code significantly less readable. > As far as I have read lists are much faster for access and iteration, is this > wrong? For random access, yes of course. For iteration, probably, although I'm not sure how much in practice. For subscription lists, when I tested things out for #6916 [1], it seems iteration had become ~25% slower when it was a generator. We may have a similar impact here, but adjusted for the savings in memory and simplification of the code I think it's worth it. [1]: https://issues.adblockplus.org/ticket/6916#Performanceimpact
https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.j... lib/filterStorage.js:373: if (this.subscriptionCount == 0) You mean to change this to `this.knownSubscriptions.size` as well? https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.j... lib/filterStorage.js:447: let subscriptions = [...this.subscriptions()].filter( On 2018/09/28 22:33:25, Manish Jethani wrote: > On 2018/09/28 08:15:49, Manish Jethani wrote: > > On 2018/09/27 18:46:59, Jon Sonesen wrote: > > > On 2018/09/27 17:42:54, Manish Jethani wrote: > > > > I think we should remove this and just add the following to the loops > below: > > > > > > > > if (subscription instanceof ExternalSubscription) > > > > continue; > > > > > > Can the iterations be consolidated as well? > > > > It doesn't look like, because all the subscriptions need to be serialized > first > > before each subscription's filters. > > > > [...] > > OK, scratch this. Let's leave it as it is (in the current patch). > > [...] I'm sorry about this once again, but I've done an actual test now (since I've been obsessing about related things lately). It seems using the `filter` method here is a really bad idea. We should just push to an array instead. let subscriptions = []; for (let subscription of this.subscriptions()) { if (!(subscription instanceof ExternalSubscription)) subscriptions.push(subscription); } This seems to give the best results. You can try it out, either by adding some before/after code within the function itself or just using the JS profiler. Otherwise you could just take my word for it. :)
On 2018/10/02 00:41:30, Manish Jethani wrote: > https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.js > File lib/filterStorage.js (right): > > https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.j... > lib/filterStorage.js:373: if (this.subscriptionCount == 0) > You mean to change this to `this.knownSubscriptions.size` as well? > > https://codereview.adblockplus.org/29886685/diff/29893581/lib/filterStorage.j... > lib/filterStorage.js:447: let subscriptions = [...this.subscriptions()].filter( > On 2018/09/28 22:33:25, Manish Jethani wrote: > > On 2018/09/28 08:15:49, Manish Jethani wrote: > > > On 2018/09/27 18:46:59, Jon Sonesen wrote: > > > > On 2018/09/27 17:42:54, Manish Jethani wrote: > > > > > I think we should remove this and just add the following to the loops > > below: > > > > > > > > > > if (subscription instanceof ExternalSubscription) > > > > > continue; > > > > > > > > Can the iterations be consolidated as well? > > > > > > It doesn't look like, because all the subscriptions need to be serialized > > first > > > before each subscription's filters. > > > > > > [...] > > > > OK, scratch this. Let's leave it as it is (in the current patch). > > > > [...] > > I'm sorry about this once again, but I've done an actual test now (since I've > been obsessing about related things lately). It seems using the `filter` method > here is a really bad idea. We should just push to an array instead. > > let subscriptions = []; > for (let subscription of this.subscriptions()) > { > if (!(subscription instanceof ExternalSubscription)) > subscriptions.push(subscription); > } > > This seems to give the best results. You can try it out, either by adding some > before/after code within the function itself or just using the JS profiler. > Otherwise you could just take my word for it. :) Yeah you're right here. Actually it makes sense as well, since the spreader would generate the whole list first, then check each element vs the generator which just evaluates at each yield right?
On 2018/10/02 15:06:12, Jon Sonesen wrote: > On 2018/10/02 00:41:30, Manish Jethani wrote: > [..] > > > This seems to give the best results. You can try it out, either by adding some > > before/after code within the function itself or just using the JS profiler. > > Otherwise you could just take my word for it. :) > > Yeah you're right here. Actually it makes sense as well, since the spreader > would generate the whole list first, then check each element vs the generator > which just evaluates at each yield right? Yeah, exactly. So the change looks good to me except these two modifications that I mentioned in the last comment.
https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.j... lib/filterStorage.js:115: for (let subscription of FilterStorage.subscriptions()) On 2018/10/01 16:12:52, Manish Jethani wrote: > On 2018/10/01 15:40:57, Jon Sonesen wrote: > > On 2018/10/01 10:40:45, Manish Jethani wrote: > > > Since this is within the FilterStorage object, maybe it's better to directly > > > access the knownSubscriptions object here? (It is private to the object, but > > > code within the object should be able to access it directly) > > > > > > So like `FilterStorage.knownSubscriptions.values()` instead of > > > `FilterStorage.subscriptions()` is what I was thinking. > > > > Yeah, I think you are right. I had the same thought while writing this but > > figured to just stick with the previous convention to see what happened in > > review. Also, convention was my only argument for not changing. Which kinda > > makes less sense now. Although (and I hadn't thought of this previously) are > we > > hurting ourselves performance wise? > > I'm not sure, but it shouldn't hurt if we just avoid an additional function call > either, unless doing so makes the code significantly less readable. > > > As far as I have read lists are much faster for access and iteration, is this > > wrong? > > For random access, yes of course. For iteration, probably, although I'm not sure > how much in practice. For subscription lists, when I tested things out for #6916 > [1], it seems iteration had become ~25% slower when it was a generator. We may > have a similar impact here, but adjusted for the savings in memory and > simplification of the code I think it's worth it. > > [1]: https://issues.adblockplus.org/ticket/6916#Performanceimpact Done. https://codereview.adblockplus.org/29886685/diff/29896565/lib/filterStorage.j... lib/filterStorage.js:373: if (this.subscriptionCount == 0) On 2018/10/01 15:40:57, Jon Sonesen wrote: > On 2018/10/01 10:40:45, Manish Jethani wrote: > > Here (and below) too I think it's probably better to just access > > `this.knownSubscriptions.size` directly. > > Acknowledged. Done.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
