Issue 29791555: Issue 6727 - Use string rather than map for single-domain filters (Closed)
|
Created:
May 26, 2018, 10:52 a.m. by Manish Jethani Modified:
June 26, 2018, 9:25 a.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionThis reduces the memory footprint of ElemHideBase subclasses by ~2 MB. Before this change it's ~12.5 MB on my machine, after the change it's ~10.5 MB.
We avoid creating unnecessary maps for single-domain cases.
Out of ~30,000 element hiding and emulation filters and exceptions in EasyList+AA, ~12,000 are single-domain cases.
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebase #Patch Set 3 : Fix ESLint error #Patch Set 4 : Use temporary Map objects #MessagesTotal messages: 11
Patch Set 1
A good catch! I think it deserves an issue, would you mind to create one? Do I correctly understand that you are talking about "~2 MB" in JS Heap? It's likely more in the "physical memory" (RES + swap). I didn't check whether all places where Filter.domains are updated, but so far it looks correct. I'm moving Dave to reviewers, I'm pretty sure he will catch if there is some flaw. If you find my comments legit I'm fine if they are addressed separately as refactoring commits. https://codereview.adblockplus.org/29791555/diff/29791556/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29791555/diff/29791556/lib/elemHide.js#new... lib/elemHide.js:76: if (typeof domains == "string") Will the refactoring of that `typeof DOMAINS == "string"` with the applying of an action for each domain significantly negatively affect performance and the memory usage? I'm also not a big fan of spreading the logic let defaultDomains = new Map([["", true]]); and domains = [[domains, true]]; among files, IMO it would be better to keep it in filter classes. https://codereview.adblockplus.org/29791555/diff/29791556/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29791555/diff/29791556/lib/filterClasses.j... lib/filterClasses.js:537: typeof this.domains != "string" && this.domains.get("")) IMO, perhaps it would be safer to rather check that `this.domains instanceof Map` instead of typeof. BTW, despite it's correct, wouldn't it be better to wrap the logical AND into the braces () in order to improve readability because I'm not sure how many people actually remember that && has a higher priority than || in JS?
On 2018/05/28 09:22:07, sergei wrote: > A good catch! I think it deserves an issue, would you mind to create one? Done. https://issues.adblockplus.org/ticket/6727 > Do I correctly understand that you are talking about "~2 MB" in JS Heap? It's > likely more in the "physical memory" (RES + swap). Yes, that was the heap, but I checked the value reported by "Memory Footprint" in the Task Manager and it's still a ~2 MB improvement. > I didn't check whether all places where Filter.domains are updated, but so far > it looks correct. I'm moving Dave to reviewers, I'm pretty sure he will catch if > there is some flaw. > > If you find my comments legit I'm fine if they are addressed separately as > refactoring commits. Thanks for the comments, I'll address them once Dave has had a chance to comment on the Trac issue. If we agree this is a good change then we can work on doing it in a better way.
Once again sorry for my slow response. I think if we can save something like the 2 megabytes you're seeing for our users then this change is worth it. Shiny star for figuring that out. I don't especially like the extra logic it requires, but it's not too bad. The actual changes here look pretty good too, if we could abstract the logic of dealing with string vs map domains that would be nice but otherwise this LGTM. https://codereview.adblockplus.org/29791555/diff/29791556/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/29791555/diff/29791556/lib/elemHide.js#new... lib/elemHide.js:76: if (typeof domains == "string") On 2018/05/28 09:22:07, sergei wrote: > I'm also not a big fan of spreading the logic ... > among files, IMO it would be better to keep it in filter classes. Agreed, if possible (without messing up performance / memory usage) that would be nice. https://codereview.adblockplus.org/29791555/diff/29791556/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29791555/diff/29791556/lib/filterClasses.j... lib/filterClasses.js:537: typeof this.domains != "string" && this.domains.get("")) On 2018/05/28 09:22:07, sergei wrote: > BTW, despite it's correct, wouldn't it be better to wrap the logical AND into > the braces () in order to improve readability because I'm not sure how many > people actually remember that && has a higher priority than || in JS? Disagree on this one, people that don't understand JavaScript's logical operators shouldn't really be messing with this code IMO.
https://codereview.adblockplus.org/29791555/diff/29791556/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29791555/diff/29791556/lib/filterClasses.j... lib/filterClasses.js:537: typeof this.domains != "string" && this.domains.get("")) On 2018/06/05 16:51:30, kzar wrote: > On 2018/05/28 09:22:07, sergei wrote: > > > BTW, despite it's correct, wouldn't it be better to wrap the logical AND into > > the braces () in order to improve readability because I'm not sure how many > > people actually remember that && has a higher priority than || in JS? > > Disagree on this one, people that don't understand JavaScript's logical > operators shouldn't really be messing with this code IMO. This is one of those things that can lead to an endless debate, so in the interest of consistency I'll leave it as it is. It seems in the rest of the code we assume knowledge of logical operator precedence. I personally do prefer to group things logically though.
On 2018/06/06 11:16:31, Manish Jethani wrote: > I personally do prefer to group things logically though. Well if you prefer to add the parenthesis I really don't mind that much. Whichever you guys prefer is OK with me.
Patch Set 2: Rebase Patch Set 3: Fix ESLint error Patch Set 4: Use temporary Map objects The only way to really maintain the abstractions is to return a temporary Map object with two entries. I'm not sure that it's worth it. What do you think?
https://codereview.adblockplus.org/29791555/diff/29791556/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29791555/diff/29791556/lib/filterClasses.j... lib/filterClasses.js:537: typeof this.domains != "string" && this.domains.get("")) On 2018/05/28 09:22:07, sergei wrote: > IMO, perhaps it would be safer to rather check that `this.domains instanceof > Map` instead of typeof. typeof is typically faster than instanceof, so where there's a choice I think it makes sense to use the former.
Patch Set 4 is a simplified version of this that keeps the changes in one file. Dave, Sergei, any further thoughts on this?
LGTM 
|
