Issue 29871555: Issue 6916 - Avoid Set object for filters with only one subscription (Closed)
|
Created:
Sept. 1, 2018, 5:12 a.m. by Manish Jethani Modified:
Sept. 4, 2018, 3:04 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionBased on https://codereview.adblockplus.org/29870577/
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Add type check to addSubscription #
Total comments: 6
MessagesTotal messages: 10
Patch Set 1 Based on https://codereview.adblockplus.org/29870577/ I'll file an issue on Monday, but the short version is that this saves ~12 MB on the heap. Out of ~82,000 filters, only 7 have more than one subscription. We are storing ~82,000 Set objects (previously arrays) in memory unnecessarily when we can just point to the one Subscription directly. To do this in a clean way we need to change the API so we encapsulate the subscriptions.
Patch Set 2: Rebase
Patch Set 3: Add type check to addSubscription
Mostly looks good just a nit or two and a question. Thanks! https://codereview.adblockplus.org/29871555/diff/29871591/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29871555/diff/29871591/lib/filterClasses.j... lib/filterClasses.js:72: * @yields {Subscription} Should this be {(Subscription|Set.<Subscription>)?} https://codereview.adblockplus.org/29871555/diff/29871591/lib/filterClasses.j... lib/filterClasses.js:105: // subscription that isn't a non-null object. the double negative in the comment is sort of confusing, perhaps "Since we use truthy checks in our logic, we must avoid adding a subscription that is null or not an object" is a bit more clear? https://codereview.adblockplus.org/29871555/diff/29871591/lib/filterClasses.j... lib/filterClasses.js:136: this._subscriptions = [...this._subscriptions][0]; Just asking, not so much a comment but this line changes _subscriptions to whichever the remaining member of the set is?
Mostly looks good just a nit or two and a question. Thanks!
https://codereview.adblockplus.org/29871555/diff/29871591/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29871555/diff/29871591/lib/filterClasses.j... lib/filterClasses.js:72: * @yields {Subscription} On 2018/09/02 17:22:50, Jon Sonesen wrote: > Should this be {(Subscription|Set.<Subscription>)?} No, the function only ever yields a single Subscription object. Note that we're using `yield*` if it's a set, so each entry in the set is yielded individually. This way the caller doesn't need to know about the internal optimization and always gets a consistent value. https://codereview.adblockplus.org/29871555/diff/29871591/lib/filterClasses.j... lib/filterClasses.js:105: // subscription that isn't a non-null object. On 2018/09/02 17:22:50, Jon Sonesen wrote: > the double negative in the comment is sort of confusing, perhaps > > "Since we use truthy checks in our logic, we must avoid adding a subscription > that is null or not an object" > > is a bit more clear? The non-null part is a bit of a distraction: the problem is that `typeof null` is also "object" in JavaScript, otherwise we could have just had the typeof check. I can remove the "non-null" in that sentence if it makes it more readable, it's obvious from the line that follows that we're checking for `null`. What do you think? https://codereview.adblockplus.org/29871555/diff/29871591/lib/filterClasses.j... lib/filterClasses.js:136: this._subscriptions = [...this._subscriptions][0]; On 2018/09/02 17:22:50, Jon Sonesen wrote: > Just asking, not so much a comment but this line changes _subscriptions to > whichever the remaining member of the set is? Yes, that's exactly what it does. We can't do `this._subscriptions[0]` because it's not an array, so basically we convert it to an array first using the spread operator.
LGTM
On 2018/09/04 14:36:11, hub wrote: > LGTM Thanks! How about the base patch? https://codereview.adblockplus.org/29870577/
On 2018/09/04 14:38:31, Manish Jethani wrote: > On 2018/09/04 14:36:11, hub wrote: > > LGTM > > Thanks! > > How about the base patch? > > https://codereview.adblockplus.org/29870577/ uh oh, I missed it. let me look at it then. 
|
