Issue 29587914: Issue 5142 - Convert Element Hiding to C++ (Closed)
DescriptionIssue 5142 - Convert Element Hiding to C++
Patch Set 1 #
Total comments: 11
Patch Set 2 : More test, cache the unconditional selectors. Various cleanup #
Total comments: 2
Patch Set 3 : A few more fixes. #Patch Set 4 : Add proper delete for filters in tests. #Patch Set 5 : Some style change following feedback from a different review. #Patch Set 6 : Added OwnedStringMap to address ownership of keys #Patch Set 7 : Rebased on master. #
Total comments: 45
Patch Set 8 : Reworked after review. #
Total comments: 14
Patch Set 9 : Re-added elemHide.js test. Fixed test failures related. Addresses review comments. #
Total comments: 2
Patch Set 10 : Fixed leaks in test. Extracted changes to string and string map. #Patch Set 11 : remove an unused #include #Patch Set 12 : defaultDomains is now in an anon namespace #Patch Set 13 : Depend on #6279. Other minor fixes. #
Total comments: 21
Patch Set 14 : Use withNAD in the test. #Patch Set 15 : Updated from feedback #Patch Set 16 : mFiltersByDomain is now an OwnedStringMap #
MessagesTotal messages: 22
It is based on some unlanded patches. I have a few more changes to do on this patch, but I wanted to get it out. https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.h File compiled/ElemHide.h (right): https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.h... compiled/ElemHide.h:55: StringMap<std::unordered_map<DependentString,ElemHideBasePtr,StringHash>> mFiltersByDomain; Using an std::unordered_map. Bu if we make StringMap movable we should be able to use it here. https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.h... compiled/ElemHide.h:86: ElemHideException* BINDINGS_EXPORTED GetException(const ElemHideBase& filter, This function is needed until ElemHideEmulation is modified to not use it. https://codereview.adblockplus.org/29587914/diff/29587915/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29587914/diff/29587915/compiled/String.h#n... compiled/String.h:195: void toUpper() I now realize I don't need this one. Will remove it. https://codereview.adblockplus.org/29587914/diff/29587915/compiled/StringMap.h File compiled/StringMap.h (right): https://codereview.adblockplus.org/29587914/diff/29587915/compiled/StringMap.... compiled/StringMap.h:25: class StringHash this is needed for the unordered_map<>. Otherwise not. https://codereview.adblockplus.org/29587914/diff/29587915/compiled/library.h File compiled/library.h (right): https://codereview.adblockplus.org/29587914/diff/29587915/compiled/library.h#... compiled/library.h:36: char16_t CharToUpper(char16_t charCode); I shall remove this one too. https://codereview.adblockplus.org/29587914/diff/29587915/compiled/library.js File compiled/library.js (right): https://codereview.adblockplus.org/29587914/diff/29587915/compiled/library.js... compiled/library.js:46: CharToUpper: function(charCode) and remove this too. https://codereview.adblockplus.org/29587914/diff/29587915/meson.build File meson.build (right): https://codereview.adblockplus.org/29587914/diff/29587915/meson.build#newcode75 meson.build:75: 'compiled/ElemHide.cpp', I'm based on top of the meson build system patch. Depending on its status, this might need to be removed from the patch. https://codereview.adblockplus.org/29587914/diff/29587915/test/elemHideEmulat... File test/elemHideEmulation.js (right): https://codereview.adblockplus.org/29587914/diff/29587915/test/elemHideEmulat... test/elemHideEmulation.js:59: let selectors = ElemHide.getSelectorsForDomain("example.com", 1); Argument 2 is the Criteria enum. I'm no sure how to represent it in the bindings.
https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.c... compiled/ElemHide.cpp:172: _ElemHide_SelectorList* ElemHide::GetUnconditionalSelectors() const there is also a more efficient way to implement this.
This looks better. Still depend on unlanded patches.
https://codereview.adblockplus.org/29587914/diff/29589658/compiled/ElemHide.h File compiled/ElemHide.h (right): https://codereview.adblockplus.org/29587914/diff/29589658/compiled/ElemHide.h... compiled/ElemHide.h:43: void push_back(ElemHideBasePtr filter) I'm doing this wrong. We are supposed to return selectors. Will fix this.
https://codereview.adblockplus.org/29587914/diff/29589658/compiled/ElemHide.h File compiled/ElemHide.h (right): https://codereview.adblockplus.org/29587914/diff/29589658/compiled/ElemHide.h... compiled/ElemHide.h:43: void push_back(ElemHideBasePtr filter) On 2017/10/26 20:53:30, hub wrote: > I'm doing this wrong. We are supposed to return selectors. Will fix this. but then FilterKeyAt() needs more than just the selector so this look like the simple way to do it.
First portion of comments mainly merely regarding the code. https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.h File compiled/ElemHide.h (right): https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.h... compiled/ElemHide.h:55: StringMap<std::unordered_map<DependentString,ElemHideBasePtr,StringHash>> mFiltersByDomain; On 2017/10/25 01:19:38, hub wrote: > Using an std::unordered_map. Bu if we make StringMap movable we should be able > to use it here. What is the overhead from the std::unordered_map? As far as I remember we didn't use it because it contributed really a lot into the generated JS code. I think we should make StringMap movable, though probably entry of the container should be movable what can also optimize resize method of the container. Maybe implement it as another issue? https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:22: return std::move(mSelectors[idx]->GetSelector()); std::move is redundant for return values. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:42: ActiveFilter::DomainMap* DefaultDomains() What about just making it as a variable? AFAIK the map object can be initialized in constructor. In addition, it should be const. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:54: auto domains = filter.GetDomains(); it should be `const auto domains`. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:77: if (!filter.As<ElemHideBase>()) `filter` type is already `ElemHideBase`, it looks redundant. If it's possible to construct `ElemHideBase` with a type which does not include the type of ElemHideBase then we should change that IMO. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:87: mExceptions[selector].push_back( with emplace_back it's not necessary to use ElemHideExceptionPtr. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:100: mFilters[text] = ElemHideBasePtr(filter.As<ElemHideBase>()); ElemHideBasePtr is not required here. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:167: if ((*iter)->IsActiveOnDomain(domain, u""_str)) should the sitekey parameter be the invalid string (DependentString)? https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:181: mUnconditionalSelectorsCache = intrusive_ptr<ElemHide_SelectorList>(new ElemHide_SelectorList, false); it woule be better to have it `new X()` with parentheses. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:185: auto filter = mFilters.find(unconditional.first); what do you think about renaming it to something reflecting that it's not a filter, it's a reference? BTW, it seems it can return a deleted bucket, we should be really careful with it everywhere... https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:196: intrusive_ptr<ElemHide_SelectorList> selectors(new ElemHide_SelectorList); it would be better to always use () or {} in new expressions, just for consistency. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:200: selectors->append(GetUnconditionalSelectors()); GetUnconditionalSelectors() returns a raw pointer to ref counted, so we should delete it after the usage. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:202: bool specificOnly = (criteria >= SPECIFIC_ONLY); I think it would be better to remove the parentheses here. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:217: if (entry.first.is_invalid()) should deleted be also filtered out? https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:233: auto nextDot = currentDomain.find('.'); the parameter should be u'.'. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.h File compiled/ElemHide.h (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.h... compiled/ElemHide.h:50: void append(const ElemHide_SelectorList* list) should it be a reference instead of a pointer? https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.h... compiled/ElemHide.h:79: ALL_MATCHING = 0, Could you please bring the comments from js file here, because it will be very difficult to understand later the idea and aim of some parts? https://codereview.adblockplus.org/29587914/diff/29631257/compiled/Map.h File compiled/Map.h (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/Map.h#newc... compiled/Map.h:236: bool empty() const What about renaming of `empty` and `size` methods because IMO they are confusing. The interface is pretty close to the interface of std library but these two methods are different because they don't reflect the actual number of stored elements, they show the number of occupied buckets where all buckets can contain deleted entries, so e.g. std::map::size() would return zero in this case. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/String.h#n... compiled/String.h:284: : String(nullptr, len, len ? READ_WRITE : INVALID) If it's not difficult could you please put the changes in String.h in a different commit? This fix is pretty important IMO and it would be good to have it in a separate commit. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/StringMap.h File compiled/StringMap.h (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/StringMap.... compiled/StringMap.h:37: class StringHash what about making it struct? https://codereview.adblockplus.org/29587914/diff/29631257/compiled/bindings/m... File compiled/bindings/main.cpp (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/bindings/m... compiled/bindings/main.cpp:151: singleton<ElemHide>("ElemHide", &ElemHide::GetInstance) Could you please change it from singleton to a class with the regular lifetime?
Patch updated. Now require https://codereview.adblockplus.org/29669631/ that was split out. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:22: return std::move(mSelectors[idx]->GetSelector()); On 2018/01/15 15:31:18, sergei wrote: > std::move is redundant for return values. Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:42: ActiveFilter::DomainMap* DefaultDomains() On 2018/01/15 15:31:17, sergei wrote: > What about just making it as a variable? AFAIK the map object can be initialized > in constructor. > In addition, it should be const. Done, but I can't make it const since we need to set it up. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:54: auto domains = filter.GetDomains(); On 2018/01/15 15:31:17, sergei wrote: > it should be `const auto domains`. this would prevent line 56 from compiling because it makes the variable (pointer type) const and not the dereferenced type. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:77: if (!filter.As<ElemHideBase>()) On 2018/01/15 15:31:16, sergei wrote: > `filter` type is already `ElemHideBase`, it looks redundant. If it's possible to > construct `ElemHideBase` with a type which does not include the type of > ElemHideBase then we should change that IMO. It is not. The bindings don't guarantee it and in the tests (`testDomainRestrictions`) we get filters of the wrong type. This ensure it doesn't happen. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:87: mExceptions[selector].push_back( On 2018/01/15 15:31:16, sergei wrote: > with emplace_back it's not necessary to use ElemHideExceptionPtr. Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:100: mFilters[text] = ElemHideBasePtr(filter.As<ElemHideBase>()); On 2018/01/15 15:31:17, sergei wrote: > ElemHideBasePtr is not required here. Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:167: if ((*iter)->IsActiveOnDomain(domain, u""_str)) On 2018/01/15 15:31:17, sergei wrote: > should the sitekey parameter be the invalid string (DependentString)? It's probably better. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:181: mUnconditionalSelectorsCache = intrusive_ptr<ElemHide_SelectorList>(new ElemHide_SelectorList, false); On 2018/01/15 15:31:16, sergei wrote: > it woule be better to have it `new X()` with parentheses. Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:185: auto filter = mFilters.find(unconditional.first); On 2018/01/15 15:31:18, sergei wrote: > what do you think about renaming it to something reflecting that it's not a > filter, it's a reference? BTW, it seems it can return a deleted bucket, we > should be really careful with it everywhere... We check the entry just below and if it has been deleted, we do nothing. Renaming `filter` to `entry`. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:196: intrusive_ptr<ElemHide_SelectorList> selectors(new ElemHide_SelectorList); On 2018/01/15 15:31:17, sergei wrote: > it would be better to always use () or {} in new expressions, just for > consistency. Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:200: selectors->append(GetUnconditionalSelectors()); On 2018/01/15 15:31:18, sergei wrote: > GetUnconditionalSelectors() returns a raw pointer to ref counted, so we should > delete it after the usage. Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:202: bool specificOnly = (criteria >= SPECIFIC_ONLY); On 2018/01/15 15:31:18, sergei wrote: > I think it would be better to remove the parentheses here. Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:217: if (entry.first.is_invalid()) On 2018/01/15 15:31:17, sergei wrote: > should deleted be also filtered out? I think so. Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:233: auto nextDot = currentDomain.find('.'); On 2018/01/15 15:31:16, sergei wrote: > the parameter should be u'.'. Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.h File compiled/ElemHide.h (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.h... compiled/ElemHide.h:50: void append(const ElemHide_SelectorList* list) On 2018/01/15 15:31:18, sergei wrote: > should it be a reference instead of a pointer? Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.h... compiled/ElemHide.h:79: ALL_MATCHING = 0, On 2018/01/15 15:31:18, sergei wrote: > Could you please bring the comments from js file here, because it will be very > difficult to understand later the idea and aim of some parts? Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/Map.h File compiled/Map.h (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/Map.h#newc... compiled/Map.h:236: bool empty() const On 2018/01/15 15:31:19, sergei wrote: > What about renaming of `empty` and `size` methods because IMO they are > confusing. The interface is pretty close to the interface of std library but > these two methods are different because they don't reflect the actual number of > stored elements, they show the number of occupied buckets where all buckets can > contain deleted entries, so e.g. std::map::size() would return zero in this > case. I'm willing to not add empty() and use size() == 0 where I need it. Then we can actually fix HashContainer() in a different patch. Done https://codereview.adblockplus.org/29587914/diff/29631257/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/String.h#n... compiled/String.h:284: : String(nullptr, len, len ? READ_WRITE : INVALID) On 2018/01/15 15:31:19, sergei wrote: > If it's not difficult could you please put the changes in String.h in a > different commit? This fix is pretty important IMO and it would be good to have > it in a separate commit. Done https://codereview.adblockplus.org/29669631/ https://codereview.adblockplus.org/29587914/diff/29631257/compiled/StringMap.h File compiled/StringMap.h (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/StringMap.... compiled/StringMap.h:37: class StringHash On 2018/01/15 15:31:19, sergei wrote: > what about making it struct? Done. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/bindings/m... File compiled/bindings/main.cpp (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/bindings/m... compiled/bindings/main.cpp:151: singleton<ElemHide>("ElemHide", &ElemHide::GetInstance) On 2018/01/15 15:31:19, sergei wrote: > Could you please change it from singleton to a class with the regular lifetime? I can. This require a few other changes, ElemHideEmulation (still in JS) to take and ElemHide parameter, at least in this patch.
So, I have not checked logic yet :) will do it next. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:54: auto domains = filter.GetDomains(); On 2018/01/16 02:57:37, hub wrote: > On 2018/01/15 15:31:17, sergei wrote: > > it should be `const auto domains`. > > this would prevent line 56 from compiling because it makes the variable (pointer > type) const and not the dereferenced type. One should use `const auto* domains = `. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:77: if (!filter.As<ElemHideBase>()) On 2018/01/16 02:57:36, hub wrote: > On 2018/01/15 15:31:16, sergei wrote: > > `filter` type is already `ElemHideBase`, it looks redundant. If it's possible > to > > construct `ElemHideBase` with a type which does not include the type of > > ElemHideBase then we should change that IMO. > > It is not. The bindings don't guarantee it and in the tests > (`testDomainRestrictions`) we get filters of the wrong type. This ensure it > doesn't happen. Acknowledged, then let's keep it as is. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:185: auto filter = mFilters.find(unconditional.first); On 2018/01/16 02:57:36, hub wrote: > On 2018/01/15 15:31:18, sergei wrote: > > what do you think about renaming it to something reflecting that it's not a > > filter, it's a reference? BTW, it seems it can return a deleted bucket, we > > should be really careful with it everywhere... > > We check the entry just below and if it has been deleted, we do nothing. > > Renaming `filter` to `entry`. It's actually correct but I thought that find (which calls find_bucket) can also return deleted entries, fortunately according to find_bucket it can return either an invalid bucket or a bucket with a payload, so checking below whether it's an invalid is enough. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/Map.h File compiled/Map.h (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/Map.h#newc... compiled/Map.h:236: bool empty() const On 2018/01/16 02:57:39, hub wrote: > On 2018/01/15 15:31:19, sergei wrote: > > What about renaming of `empty` and `size` methods because IMO they are > > confusing. The interface is pretty close to the interface of std library but > > these two methods are different because they don't reflect the actual number > of > > stored elements, they show the number of occupied buckets where all buckets > can > > contain deleted entries, so e.g. std::map::size() would return zero in this > > case. > > I'm willing to not add empty() and use size() == 0 where I need it. Then we can > actually fix HashContainer() in a different patch. > > Done As I see it's really used as !((filter.GetDomains() && filter.GetDomains()->size()) || mExceptions.find(filter.GetSelector())) in ElemHide::Add. Can there be something wrong when the size is positive but there are actually nothing because the entry was removed? I have not checked the whole logic of the code in details yet. https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:38: ActiveFilter::DomainMap ElemHide::defaultDomains; It should be just namespace { const ActiveFilter::DomainMap defaultDomains = {{ActiveFilter::DEFAULT_DOMAIN, true}}; } https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:47: for (auto& domain : *domains) it should be `const auto&`. https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:189: auto selector = GetUnconditionalSelectors(); what about using of intrusive_ptr? https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:209: if (entry.first.is_invalid() || entry.first.is_deleted()) I have added https://issues.adblockplus.org/ticket/6280 and https://issues.adblockplus.org/ticket/6281
It seems test/elemHide.js is not added, I think it's very essential here because the logic is really non-trivial (at least for me), could you please add it? https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:69: if (filter.mType == Filter::Type::ELEMHIDEEXCEPTION) what about if (auto exceptionFilter = filter.As<ElemHideException>()) { use exceptionFilter here. } and below too. Can it happen that Filter::Type::ELEMHIDEEXCEPTION is just a mask and a simple comparison is not enough? https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:132: for (auto domain : *domains) Can domains be nullptr?
On 2018/01/16 18:05:28, sergei wrote: > It seems test/elemHide.js is not added, I think it's very essential here because > the logic is really non-trivial (at least for me), could you please add it? That's a short sight on my part. I actually missed that there was that test too (in addition to the elemHideEmulation.js). Will update with the test added.
Sorry about that missing, led to several fixes. Patch now updated. https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.h File compiled/ElemHide.h (right): https://codereview.adblockplus.org/29587914/diff/29587915/compiled/ElemHide.h... compiled/ElemHide.h:55: StringMap<std::unordered_map<DependentString,ElemHideBasePtr,StringHash>> mFiltersByDomain; On 2018/01/15 15:31:15, sergei wrote: > On 2017/10/25 01:19:38, hub wrote: > > Using an std::unordered_map. Bu if we make StringMap movable we should be able > > to use it here. > > What is the overhead from the std::unordered_map? As far as I remember we didn't > use it because it contributed really a lot into the generated JS code. > I think we should make StringMap movable, though probably entry of the container > should be movable what can also optimize resize method of the container. Maybe > implement it as another issue? File https://issues.adblockplus.org/ticket/6279 to make Map<> moveable. https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29631257/compiled/ElemHide.c... compiled/ElemHide.cpp:54: auto domains = filter.GetDomains(); On 2018/01/16 16:43:57, sergei wrote: > On 2018/01/16 02:57:37, hub wrote: > > On 2018/01/15 15:31:17, sergei wrote: > > > it should be `const auto domains`. > > > > this would prevent line 56 from compiling because it makes the variable > (pointer > > type) const and not the dereferenced type. > > One should use `const auto* domains = `. Done. https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:38: ActiveFilter::DomainMap ElemHide::defaultDomains; On 2018/01/16 16:43:59, sergei wrote: > It should be just > namespace > { > const ActiveFilter::DomainMap defaultDomains = {{ActiveFilter::DEFAULT_DOMAIN, > true}}; > } Done. https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:47: for (auto& domain : *domains) On 2018/01/16 16:43:59, sergei wrote: > it should be `const auto&`. Done. https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:69: if (filter.mType == Filter::Type::ELEMHIDEEXCEPTION) On 2018/01/16 18:05:27, sergei wrote: > what about > if (auto exceptionFilter = filter.As<ElemHideException>()) > { > use exceptionFilter here. > } > and below too. Done. > > Can it happen that Filter::Type::ELEMHIDEEXCEPTION is just a mask and a simple > comparison is not enough? Filter::Type::ELEMHIDEEXCEPTION is not a mask, but Filter::As<> should be enough. https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:132: for (auto domain : *domains) On 2018/01/16 18:05:27, sergei wrote: > Can domains be nullptr? Anything can happen. I check it elsewhere too. Done https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:189: auto selector = GetUnconditionalSelectors(); On 2018/01/16 16:43:59, sergei wrote: > what about using of intrusive_ptr? Done. https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:209: if (entry.first.is_invalid() || entry.first.is_deleted()) On 2018/01/16 16:43:59, sergei wrote: > I have added https://issues.adblockplus.org/ticket/6280 and > https://issues.adblockplus.org/ticket/6281 Acknowledged.
Sorry for the delay, I have a couple of comments which are worthy already. Tests are failing BTW. Since it takes really too long I would propose to extract changes in Map, String, StringMap, they are ready for landing IMO. https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:38: ActiveFilter::DomainMap ElemHide::defaultDomains; On 2018/01/19 02:11:03, hub wrote: > On 2018/01/16 16:43:59, sergei wrote: > > It should be just > > namespace > > { > > const ActiveFilter::DomainMap defaultDomains = > {{ActiveFilter::DEFAULT_DOMAIN, > > true}}; > > } > > Done. Do we really need it not in the anonymous namespace? https://codereview.adblockplus.org/29587914/diff/29673635/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29587914/diff/29673635/test/elemHide.js#ne... test/elemHide.js:258: elemHide.add(Filter.fromText("##test")); there is a memory leak, Filter.fromText should be release after the usage. E.g. see how withNAD is used in https://codereview.adblockplus.org/29548581/diff/29548701/test/subscriptionCl..., where withNAD is defined in https://codereview.adblockplus.org/29548581/diff/29548701/test/_test-utils.js.... Do you think it makes sense to extract withNAD to a separate review now?
On 2018/01/22 15:40:08, sergei wrote: > Sorry for the delay, I have a couple of comments which are worthy already. > > Tests are failing BTW. How are they failing. They all pass here. > > Since it takes really too long I would propose to extract changes in Map, > String, StringMap, they are ready for landing IMO. OK. I'll see about doing this.
On 2018/01/22 16:00:47, hub wrote: > > Since it takes really too long I would propose to extract changes in Map, > > String, StringMap, they are ready for landing IMO. > > OK. I'll see about doing this. https://codereview.adblockplus.org/29676711 https://codereview.adblockplus.org/29676714 https://codereview.adblockplus.org/29676717 https://codereview.adblockplus.org/29676720
https://codereview.adblockplus.org/29587914/diff/29673635/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29587914/diff/29673635/test/elemHide.js#ne... test/elemHide.js:258: elemHide.add(Filter.fromText("##test")); On 2018/01/22 15:40:07, sergei wrote: > there is a memory leak, Filter.fromText should be release after the usage. > E.g. see how withNAD is used in > https://codereview.adblockplus.org/29548581/diff/29548701/test/subscriptionCl..., > where withNAD is defined in > https://codereview.adblockplus.org/29548581/diff/29548701/test/_test-utils.js.... > Do you think it makes sense to extract withNAD to a separate review now? This is the kind of changes that are self contained so yeah it could make sense to submite them separately. Makes for smaller patches. Will fix the leak here.
https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29669634/compiled/ElemHide.c... compiled/ElemHide.cpp:38: ActiveFilter::DomainMap ElemHide::defaultDomains; On 2018/01/22 15:40:07, sergei wrote: > On 2018/01/19 02:11:03, hub wrote: > > On 2018/01/16 16:43:59, sergei wrote: > > > It should be just > > > namespace > > > { > > > const ActiveFilter::DomainMap defaultDomains = > > {{ActiveFilter::DEFAULT_DOMAIN, > > > true}}; > > > } > > > > Done. > > Do we really need it not in the anonymous namespace? Indeed we don't. Done
just few things https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:40: const ActiveFilter::DomainMap defaultDomains = { Could you please add indentation here? https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:43: } and a new line here. https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:145: } Just wonder whether we should also remove entries for domains with no filters. https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:163: if ((*iter)->IsActiveOnDomain(domain, DependentString())) What about adding the sitekey as a function parameter with the default value DependentString()? https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:163: if ((*iter)->IsActiveOnDomain(domain, DependentString())) I would recommend to add JIC a test that *iter is not a nullptr, despite it should never happen. https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:184: auto entry = mFilters.find(unconditional.second); What about storing intrusive_ptr<Filter> in mUnconditionalSelectors instead of DependenentString from a filter text? In this case there will not be the necessity in searching among mFilters, the memory usage will be less, because the size of DependentString is bigger than intrusive_ptr and it will be less dangerous in regards that a filter can be deleted but the DependentString from its text is still somewhere here. https://codereview.adblockplus.org/29587914/diff/29678627/lib/elemHideEmulati... File lib/elemHideEmulation.js (right): https://codereview.adblockplus.org/29587914/diff/29678627/lib/elemHideEmulati... lib/elemHideEmulation.js:65: getRulesForDomain(domain, elemHide) I wonder whether the order of parameters should be "elemHide, domain" but it's not really important. https://codereview.adblockplus.org/29587914/diff/29678627/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29587914/diff/29678627/test/elemHide.js#ne... test/elemHide.js:86: addFilter("~foo.example.com,example.com##foo"); I would like to restructure this test because at some point it's already not clear what the current state is, but in another commit. https://codereview.adblockplus.org/29587914/diff/29678627/test/elemHide.js#ne... test/elemHide.js:264: exports.testZeroFilterKey = function(test) Could you please rename the test, there is no key, so it's confusing.
https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:40: const ActiveFilter::DomainMap defaultDomains = { On 2018/01/25 14:52:40, sergei wrote: > Could you please add indentation here? Done. https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:43: } On 2018/01/25 14:52:39, sergei wrote: > and a new line here. Done. https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:145: } On 2018/01/25 14:52:40, sergei wrote: > Just wonder whether we should also remove entries for domains with no filters. list is StringMap<> and erase() on it doesn't decrease size(). Until we fix this, we can't know. https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:163: if ((*iter)->IsActiveOnDomain(domain, DependentString())) On 2018/01/25 14:52:40, sergei wrote: > I would recommend to add JIC a test that *iter is not a nullptr, despite it > should never happen. Done. https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:163: if ((*iter)->IsActiveOnDomain(domain, DependentString())) On 2018/01/25 14:52:41, sergei wrote: > What about adding the sitekey as a function parameter with the default value > DependentString()? Good idea. Done. https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:184: auto entry = mFilters.find(unconditional.second); On 2018/01/25 14:52:39, sergei wrote: > What about storing intrusive_ptr<Filter> in mUnconditionalSelectors instead of > DependenentString from a filter text? In this case there will not be the > necessity in searching among mFilters, the memory usage will be less, because > the size of DependentString is bigger than intrusive_ptr and it will be less > dangerous in regards that a filter can be deleted but the DependentString from > its text is still somewhere here. OK https://codereview.adblockplus.org/29587914/diff/29678627/lib/elemHideEmulati... File lib/elemHideEmulation.js (right): https://codereview.adblockplus.org/29587914/diff/29678627/lib/elemHideEmulati... lib/elemHideEmulation.js:65: getRulesForDomain(domain, elemHide) On 2018/01/25 14:52:41, sergei wrote: > I wonder whether the order of parameters should be "elemHide, domain" but it's > not really important. Acknowledged. https://codereview.adblockplus.org/29587914/diff/29678627/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/29587914/diff/29678627/test/elemHide.js#ne... test/elemHide.js:86: addFilter("~foo.example.com,example.com##foo"); On 2018/01/25 14:52:41, sergei wrote: > I would like to restructure this test because at some point it's already not > clear what the current state is, but in another commit. it has been refactored to use withNAD() already, which is a start. https://codereview.adblockplus.org/29587914/diff/29678627/test/elemHide.js#ne... test/elemHide.js:264: exports.testZeroFilterKey = function(test) On 2018/01/25 14:52:41, sergei wrote: > Could you please rename the test, there is no key, so it's confusing. Done.
https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:145: } On 2018/01/25 16:55:08, hub wrote: > On 2018/01/25 14:52:40, sergei wrote: > > Just wonder whether we should also remove entries for domains with no filters. > > list is StringMap<> and erase() on it doesn't decrease size(). Until we fix > this, we can't know. I think we should remove it. Despite it's not supported now, it makes sense to not "leak" the memory for domains which are not used anymore, although it can be a rare case. In addition to that I think there is a tricky issue. Let's say we add a `filter1` with "domain.value", then we add `filter2` with the the same "domain-value", then we remove `filter1`, so the key, which is used in the mFiltersByDomain, which is a `DependentString` pointing to a string held by `filter1`, so it is a dangling `DependentString` now and when it is used next time in `Map`, in particular as `entry->equals(some-key)` there will be access violation. Could we do something with this?
Updated patch. Require https://codereview.adblockplus.org/29680706/ to work. https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:145: } On 2018/01/26 15:48:47, sergei wrote: > > list is StringMap<> and erase() on it doesn't decrease size(). Until we fix > > this, we can't know. > > I think we should remove it. Despite it's not supported now, it makes sense to > not "leak" the memory for domains which are not used anymore, although it can be > a rare case. erase will release the filter (it resets to the default value). When we fix StringMap we can choose to remove the entry in mFiltersByDomain. > > In addition to that I think there is a tricky issue. Let's say we add a > `filter1` with "domain.value", then we add `filter2` with the the same > "domain-value", then we remove `filter1`, so the key, which is used in the > mFiltersByDomain, which is a `DependentString` pointing to a string held by > `filter1`, so it is a dangling `DependentString` now and when it is used next > time in `Map`, in particular as `entry->equals(some-key)` there will be access > violation. Could we do something with this? Good call. I should turn them into OwnedStringMap. I'll need https://codereview.adblockplus.org/29680706/ for this to work though.
LGTM https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.cpp File compiled/ElemHide.cpp (right): https://codereview.adblockplus.org/29587914/diff/29678627/compiled/ElemHide.c... compiled/ElemHide.cpp:145: } On 2018/01/26 20:41:59, hub wrote: > On 2018/01/26 15:48:47, sergei wrote: > > > > > list is StringMap<> and erase() on it doesn't decrease size(). Until we fix > > > this, we can't know. > > > > I think we should remove it. Despite it's not supported now, it makes sense to > > not "leak" the memory for domains which are not used anymore, although it can > be > > a rare case. > > erase will release the filter (it resets to the default value). When we fix > StringMap we can choose to remove the entry in mFiltersByDomain. > > > > > In addition to that I think there is a tricky issue. Let's say we add a > > `filter1` with "domain.value", then we add `filter2` with the the same > > "domain-value", then we remove `filter1`, so the key, which is used in the > > mFiltersByDomain, which is a `DependentString` pointing to a string held by > > `filter1`, so it is a dangling `DependentString` now and when it is used next > > time in `Map`, in particular as `entry->equals(some-key)` there will be access > > violation. Could we do something with this? > > Good call. > I should turn them into OwnedStringMap. OK, I also thought that maybe we could just update the key (DependentString) to the first filter from the "value" and if the size of "value" is zero then just remove the entry, but the approach with OwnedStringMap looks also fine because it seems harder to make a mistake :). > > I'll need https://codereview.adblockplus.org/29680706/ for this to work though. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
