Issue 5150730151264256: Issue 354 - Avoid a string copy that is unnecessary most of the time (Closed)
|
Created:
April 4, 2014, 3:07 p.m. by tschuster Modified:
April 24, 2014, 10:46 a.m. Visibility:
Public. |
DescriptionAvoid a string copy that is unnecessary most of the time
Patch Set 1 #
Total comments: 1
Patch Set 2 : Avoid toUpperCase for RegexpFilter #
Total comments: 1
Patch Set 3 : Added domainSourceIsUpperCase #MessagesTotal messages: 13
Unnecessary or not - without this code the isActiveOnDomain() and isActiveOnlyOnDomain() methods are broken.
http://codereview.adblockplus.org/5150730151264256/diff/5629499534213120/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/5150730151264256/diff/5629499534213120/lib/... lib/filterClasses.js:315: let list = this.domainSource.toUpperCase().split(this.domainSeparator); I see, I didn't realize what this was doing. The domains coming from the RegExpFilter constructor will already be uppercased - I guess that this operation will be wasteful then. Maybe we should add ActiveFilter.domainsUpperCased flag or something like this to only call toUpperCase() for element hiding filters?
On 2014/04/08 12:10:05, Wladimir Palant wrote: > http://codereview.adblockplus.org/5150730151264256/diff/5629499534213120/lib/... > File lib/filterClasses.js (right): > > http://codereview.adblockplus.org/5150730151264256/diff/5629499534213120/lib/... > lib/filterClasses.js:315: let list = > this.domainSource.toUpperCase().split(this.domainSeparator); > I see, I didn't realize what this was doing. > > The domains coming from the RegExpFilter constructor will already be uppercased > - I guess that this operation will be wasteful then. Maybe we should add > ActiveFilter.domainsUpperCased flag or something like this to only call > toUpperCase() for element hiding filters? Can we do something like if (!(.. instanceof RegExpFilter)) { toUpperCase() } ?
http://codereview.adblockplus.org/5150730151264256/diff/5685265389584384/lib/... File lib/filterClasses.js (right): http://codereview.adblockplus.org/5150730151264256/diff/5685265389584384/lib/... lib/filterClasses.js:316: if (!(this instanceof RegExpFilter)) { I'd rather not hardcode the RegExpFilter class here. IMHO it is better to use a property like domainSourceUppercased that is true for RegExpFilter but false for ElemHideFilter - the same way we do it with domainSeparator property for example.
On 2014/04/10 18:43:38, Wladimir Palant wrote: > http://codereview.adblockplus.org/5150730151264256/diff/5685265389584384/lib/... > File lib/filterClasses.js (right): > > http://codereview.adblockplus.org/5150730151264256/diff/5685265389584384/lib/... > lib/filterClasses.js:316: if (!(this instanceof RegExpFilter)) { > I'd rather not hardcode the RegExpFilter class here. IMHO it is better to use a > property like domainSourceUppercased that is true for RegExpFilter but false for > ElemHideFilter - the same way we do it with domainSeparator property for > example. Actually just leave all this away, this toUpperCase call is really cheap, considering that we do .split right after. Adding a new property is a bad idea, because we pay for that memory on every Filter, which is something I am trying to actively avoid.
On 2014/04/10 19:19:53, tschuster wrote: > Adding a new property is a bad idea, because we pay for that memory on every Filter Even for a properly on the shared prototype?
On 2014/04/11 05:40:16, Wladimir Palant wrote:
> On 2014/04/10 19:19:53, tschuster wrote:
> > Adding a new property is a bad idea, because we pay for that memory on every
> Filter
>
> Even for a properly on the shared prototype?
Nope. What do you think of |isDomainAlreadyUpperCase: function() { }| ?
On 2014/04/11 10:15:09, tschuster wrote:
> Nope. What do you think of |isDomainAlreadyUpperCase: function() { }| ?
Don't get it - so can we have an additional property on the prototype or not? If
it's the former than it should really be a boolean property rather than a
method. If it is the latter then I guess a function won't help. And even if
SpiderMonkey treats functions differently then a getter should have the same
effect as a function I guess.
It's not about the domain but rather domainSource so the property name should be
something like isDomainSourceUpperCase.
Added domainSourceIsUpperCase
Sorry for being unclear. Properties just on the prototype is of course not a problem. (aside: Redefining them in the constructor however would be, because they would start living on the instance)
LGTM 
|
