Issue 29900557: Issue 7016 - Convert serialization functions into generators (Closed)
|
Created:
Oct. 3, 2018, 11:47 p.m. by Jon Sonesen Modified:
Oct. 24, 2018, 5:24 p.m. Visibility:
Public. |
DescriptionIssue 7016 - Convert serialization functions into generators
Patch Set 1 #Patch Set 2 : Remove unelated change #
Total comments: 39
Patch Set 3 : Address PS2 Comments #
Total comments: 10
Patch Set 4 : Actually Address PS2 Comments #
Total comments: 11
Patch Set 5 : Address PS4 Comments #
Total comments: 7
Patch Set 6 : Address PS5 Comments #
Total comments: 5
Patch Set 7 : Address Nits #Patch Set 8 : Actually address nits #
MessagesTotal messages: 36
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:467: yield ""; I think we still need the blank line. https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:147: *serializeFilters() This might be a good place to introduce a generic map function, as described in https://issues.adblockplus.org/ticket/6434 function* map(iterable, callback, thisArg) { for (let item of iterable) yield callback.call(thisArg, item); } We could put this is in `lib/coreUtils.js`. Then this function would look like this: *serializeFilters() { yield* map(this.filters, ({text}) => text.replace(/\[/g, "\\[")); } On the other hand, the whole point of this exercise is to improve the performance. The reason I think it might be OK to do this here is that we're already calling `String.prototype.replace` so it's just an additional function call in the stack, but if you decide to go down this route you would have to profile this to make sure at least it doesn't perform any worse. You could also just forget about it since it's not exactly related to the change we're trying to make here (it's just a thought I had so I thought I'd share it in this context). https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:288: yield "defaults=" + Nit: Since there is no longer a function call here, we should align the string concatenation like so: yield "defaults=" + this.defaults.filter( type => SpecialSubscription.defaultsMap.has(type) ).join(" "); Also it might be nice to get rid of the call to `Array.prototype.filter` here (but then again we might be getting rid of default subscription groups anyway, see https://issues.adblockplus.org/ticket/6859). https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:431: yield new Error( This would still be a throw.
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:472: buf.push("", "[Subscription filters]"); We would have to yield a blank line here before yielding "[Subscription filters]" https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:468: if (subscription.filters.length) One thing worth noting here as that before this change the serialization of a subscription was an atomic operation. This means the entire subscription was added to a buffer and _then_ each entry was yielded out. After this change, it would be possible for the subscription to be modified while it is being serialized. This could have POTENTIALLY DISASTROUS consequences, like the generation of a patterns.ini file that the INI parser is unable to read (thus causing Adblock Plus to no longer function). I'd like to take my time to think about this, but at the very least it, since it is now possible for the number of filters to go from non-zero to zero between this length check here and the actual serialization of the filters, this is clearly redundant. We can remove this check here. At the same time, let's make sure that INI parser is able to read a file with a "[Subscription filters]" section containing _no_ filters. In general we really, really, really have to make sure that this function is never able to generate a patterns.ini that the INI parser is unable to parse.
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/04 04:08:59, Manish Jethani wrote: > In general we really, really, really have to make sure that this function is > never able to generate a patterns.ini that the INI parser is unable to parse. Additionally, it should never be possible for this function to generate a patterns.ini that the INI parser can parse successfully but that still results in objects (subscriptions and filters) that are in an _inconsistent_ state (which may cause errors elsewhere). So this change is not as simple as it may look like on the surface. We are definitely complicating things here, but I think we could do this in a proper way. We will definitely have to add some more tests.
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/04 04:17:06, Manish Jethani wrote: > On 2018/10/04 04:08:59, Manish Jethani wrote: > > In general we really, really, really have to make sure that this function is > > never able to generate a patterns.ini that the INI parser is unable to parse. > > Additionally, it should never be possible for this function to generate a > patterns.ini that the INI parser can parse successfully but that still results > in objects (subscriptions and filters) that are in an _inconsistent_ state > (which may cause errors elsewhere). Alright, some more thinking about this. Let's do this in two steps. In the first step, let's do this: yield [...subscription.serialize()]; Instead of this: yield* subscription.serialize(); This cancels out some of the benefit of this change, but it still makes the code more readable and I have a hunch that it will still perform better. In the next step (another patch, another time), let's carefully evaluate the consequences of generating an object's serialized representation line by line. So let's not change the behavior of the `exportData` function as part of this patch. Now based on everything above, we should roll the "[Subscription filters]" part into `Subscription.prototype.serialize`, which can take an optional `withFilters` (boolean) parameter and do this part only if it is true (it should be false for `Subscription.prototype.toString`).
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:467: yield ""; On 2018/10/04 03:37:12, Manish Jethani wrote: > I think we still need the blank line. Oh, it seems blank lines are really not necessary, the tests seem to pass for me. Can you look into `lib/iniParser.js` to confirm that this is the case? (Note: just because the tests pass doesn't mean it's backwards compatible behavior, we'll have to make sure we can still read any patterns.ini files generated by the current production version of this code.)
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/04 04:36:23, Manish Jethani wrote: > Alright, some more thinking about this. Let's do this in two steps. In the first > step, let's do this: > > yield [...subscription.serialize()]; Sorry, I mean `yield*` here.
https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:431: yield new Error( On 2018/10/04 03:37:12, Manish Jethani wrote: > This would still be a throw. I see, you're getting an error from ESLint. I hate ESLint. You can just add the following above the `throw` statement: // This is just to make ESLint happy. yield; https://codereview.adblockplus.org/29900557/diff/29900565/test/filterClasses.js File test/filterClasses.js (left): https://codereview.adblockplus.org/29900557/diff/29900565/test/filterClasses.... test/filterClasses.js:199: let buffer = []; We should just change this to: let buffer = [...filter.serialize()]; And leave the rest of it as it is.
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/04 05:10:06, Manish Jethani wrote: > On 2018/10/04 04:36:23, Manish Jethani wrote: > > Alright, some more thinking about this. Let's do this in two steps. In the > first > > step, let's do this: > > > > yield [...subscription.serialize()]; > > Sorry, I mean `yield*` here. I profiled this. It performs slightly better than what we have right now. If we change it to `yield* subscription.serialize()` then it performs significantly better. Again, not surprising, but it's good to verify this. If you'd like to try it out, reload the extension, open the DevTools for the background page, go to the Performance tab and start profiling. Then run this script, which will open up the Alexa Top 50: #!/bin/bash for domain in $(curl https://www.alexa.com/topsites | sed -nE 's/.*\/siteinfo\/([^"]*).*/\1/p') do open -a 'Google Chrome' "http://$domain" done Stop profiling after approximately one minute, then go to the Bottom-up tab at the bottom of the window (default layout) and search for "exportData".
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:467: yield ""; On 2018/10/04 04:50:11, Manish Jethani wrote: > On 2018/10/04 03:37:12, Manish Jethani wrote: > > I think we still need the blank line. > > Oh, it seems blank lines are really not necessary, the tests seem to pass for > me. Can you look into `lib/iniParser.js` to confirm that this is the case? > (Note: just because the tests pass doesn't mean it's backwards compatible > behavior, we'll have to make sure we can still read any patterns.ini files > generated by the current production version of this code.) I thought that this was iniparser processes each line independently and if it receives a blank line it basically acts like a pass and doesn't do anything. Perhaps I am mistaken? https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:472: buf.push("", "[Subscription filters]"); On 2018/10/04 04:08:59, Manish Jethani wrote: > We would have to yield a blank line here before yielding "[Subscription > filters]" See my comment above :) https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/04 06:10:15, Manish Jethani wrote: > On 2018/10/04 05:10:06, Manish Jethani wrote: > > On 2018/10/04 04:36:23, Manish Jethani wrote: > > > Alright, some more thinking about this. Let's do this in two steps. In the > > first > > > step, let's do this: > > > > > > yield [...subscription.serialize()]; > > > > Sorry, I mean `yield*` here. > > I profiled this. It performs slightly better than what we have right now. > > If we change it to `yield* subscription.serialize()` then it performs > significantly better. Again, not surprising, but it's good to verify this. > > If you'd like to try it out, reload the extension, open the DevTools for the > background page, go to the Performance tab and start profiling. Then run this > script, which will open up the Alexa Top 50: > > #!/bin/bash > for domain in $(curl https://www.alexa.com/topsites | sed -nE > 's/.*\/siteinfo\/([^"]*).*/\1/p') > do > open -a 'Google Chrome' "http://$domain" > done > > Stop profiling after approximately one minute, then go to the Bottom-up tab at > the bottom of the window (default layout) and search for "exportData". So, I had similar concerns. But figured we can hash it out here, and this version was more 'to spec' in my mind. I actually agree that this is a pretty huge change, however doing the change which is slightly better performance wise with plans to later make the larger change seems like it may be more of a delay than anything since we would still need to address the same issue we face here in the future. So, I think it's worth it to just take the time to test and properly implement the full change. However, I do also realize this change makes the code a bit harder to follow and so we should consider how much that matters here. We are trying to optimize core as much as possible (for good reason) and as long as it makes sense, then I am up for it. Also, if you insist we just hold off for now and use your suggestion I am perfectly happy to do so, this was just my two rupees worth of opinion. https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:147: *serializeFilters() On 2018/10/04 03:37:12, Manish Jethani wrote: > This might be a good place to introduce a generic map function, as described in > https://issues.adblockplus.org/ticket/6434 > > function* map(iterable, callback, thisArg) > { > for (let item of iterable) > yield callback.call(thisArg, item); > } > > We could put this is in `lib/coreUtils.js`. > > Then this function would look like this: > > *serializeFilters() > { > yield* map(this.filters, ({text}) => text.replace(/\[/g, "\\[")); > } > > On the other hand, the whole point of this exercise is to improve the > performance. The reason I think it might be OK to do this here is that we're > already calling `String.prototype.replace` so it's just an additional function > call in the stack, but if you decide to go down this route you would have to > profile this to make sure at least it doesn't perform any worse. > > You could also just forget about it since it's not exactly related to the change > we're trying to make here (it's just a thought I had so I thought I'd share it > in this context). This most likely will turn into a rather large change, and I am pretty indifferent here. I think I am leaning towards not worrying about it here since it will be already a lot of changes. But I will still profile it, if it's not a huge difference I still might go for it. https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:288: yield "defaults=" + On 2018/10/04 03:37:12, Manish Jethani wrote: > Nit: Since there is no longer a function call here, we should align the string > concatenation like so: > > yield "defaults=" + > this.defaults.filter( > type => SpecialSubscription.defaultsMap.has(type) > ).join(" "); > > Also it might be nice to get rid of the call to `Array.prototype.filter` here > (but then again we might be getting rid of default subscription groups anyway, > see https://issues.adblockplus.org/ticket/6859). Acknowledged. https://codereview.adblockplus.org/29900557/diff/29900565/test/filterClasses.js File test/filterClasses.js (left): https://codereview.adblockplus.org/29900557/diff/29900565/test/filterClasses.... test/filterClasses.js:199: let buffer = []; On 2018/10/04 05:30:00, Manish Jethani wrote: > We should just change this to: > > let buffer = [...filter.serialize()]; > > And leave the rest of it as it is. Acknowledged.
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterClasses.j... lib/filterClasses.js:151: yield "[Filter]"; Let's extract the value of the text property first: let {text} = this; yield "[Filter]"; yield "text=" + text; https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterClasses.j... lib/filterClasses.js:619: if (this._disabled || this._hitCount || this._lastHit) Similarly, let's extract the values here first: let {_disabled, _hitCount, _lastHit} = this; The reason for doing this is that the values being serialized should be consistent. Inconsistent values would be, for example, when hit count is 0 but last hit is set. https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:467: yield ""; On 2018/10/06 00:06:32, Jon Sonesen wrote: > On 2018/10/04 04:50:11, Manish Jethani wrote: > > On 2018/10/04 03:37:12, Manish Jethani wrote: > > > I think we still need the blank line. > > > > Oh, it seems blank lines are really not necessary, the tests seem to pass for > > me. Can you look into `lib/iniParser.js` to confirm that this is the case? > > (Note: just because the tests pass doesn't mean it's backwards compatible > > behavior, we'll have to make sure we can still read any patterns.ini files > > generated by the current production version of this code.) > > I thought that this was iniparser processes each line independently and if it > receives a blank line it basically acts like a pass and doesn't do anything. > Perhaps I am mistaken? Yeah, blank lines are ignored [1] [1]: https://github.com/adblockplus/adblockpluscore/blob/9fba045086ff73e3986fac2fe... https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:472: buf.push("", "[Subscription filters]"); On 2018/10/06 00:06:34, Jon Sonesen wrote: > On 2018/10/04 04:08:59, Manish Jethani wrote: > > We would have to yield a blank line here before yielding "[Subscription > > filters]" > > See my comment above :) Acknowledged. https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/06 00:06:32, Jon Sonesen wrote: > On 2018/10/04 06:10:15, Manish Jethani wrote: > > On 2018/10/04 05:10:06, Manish Jethani wrote: > > > On 2018/10/04 04:36:23, Manish Jethani wrote: > > > > Alright, some more thinking about this. Let's do this in two steps. In the > > > first > > > > step, let's do this: > > > > > > > > yield [...subscription.serialize()]; > > > > > > Sorry, I mean `yield*` here. > > > > I profiled this. It performs slightly better than what we have right now. > > > > If we change it to `yield* subscription.serialize()` then it performs > > significantly better. Again, not surprising, but it's good to verify this. > > > > If you'd like to try it out, reload the extension, open the DevTools for the > > background page, go to the Performance tab and start profiling. Then run this > > script, which will open up the Alexa Top 50: > > > > #!/bin/bash > > for domain in $(curl https://www.alexa.com/topsites | sed -nE > > 's/.*\/siteinfo\/([^"]*).*/\1/p') > > do > > open -a 'Google Chrome' "http://$domain" > > done > > > > Stop profiling after approximately one minute, then go to the Bottom-up tab at > > the bottom of the window (default layout) and search for "exportData". > > So, I had similar concerns. But figured we can hash it out here, and this > version was more 'to spec' in my mind. > > I actually agree that this is a pretty huge change, however doing the change > which is slightly better performance wise with plans to later make the larger > change seems like it may be more of a delay than anything since we would still > need to address the same issue we face here in the future. So, I think it's > worth it to just take the time to test and properly implement the full change. > > However, I do also realize this change makes the code a bit harder to follow and > so we should consider how much that matters here. We are trying to optimize core > as much as possible (for good reason) and as long as it makes sense, then I am > up for it. > > Also, if you insist we just hold off for now and use your suggestion I am > perfectly happy to do so, this was just my two rupees worth of opinion. Alright, let's do this properly. So the strategy is to take a snapshot of the object right at the beginning of the serialize function, which makes it just like the current implementation in terms of serializing a consistent state of the object. let {prop1, prop2, prop3} = this; if (prop1) yield prop1; if (prop2) yield prop2; if (prop3) yield prop3; This way, if the value of a property changes in between yield statements, it would still yield the old value. https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:134: yield "[Subscription]"; Let's extract the values first: let {url, type, _title, _fixedTitle, _disabled} = this; This gives us a "snapshot" of the object at the time the function is first called, which means we always serialize a consistent state of the object. https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:147: *serializeFilters() On 2018/10/06 00:06:34, Jon Sonesen wrote: > On 2018/10/04 03:37:12, Manish Jethani wrote: > > This might be a good place to introduce a generic map function, as described > in > > https://issues.adblockplus.org/ticket/6434 > > > > function* map(iterable, callback, thisArg) > > { > > for (let item of iterable) > > yield callback.call(thisArg, item); > > } > > > > We could put this is in `lib/coreUtils.js`. > > > > Then this function would look like this: > > > > *serializeFilters() > > { > > yield* map(this.filters, ({text}) => text.replace(/\[/g, "\\[")); > > } > > > > On the other hand, the whole point of this exercise is to improve the > > performance. The reason I think it might be OK to do this here is that we're > > already calling `String.prototype.replace` so it's just an additional function > > call in the stack, but if you decide to go down this route you would have to > > profile this to make sure at least it doesn't perform any worse. > > > > You could also just forget about it since it's not exactly related to the > change > > we're trying to make here (it's just a thought I had so I thought I'd share it > > in this context). > > This most likely will turn into a rather large change, and I am pretty > indifferent here. I think I am leaning towards not worrying about it here since > it will be already a lot of changes. But I will still profile it, if it's not a > huge difference I still might go for it. Acknowledged. https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:285: yield* Subscription.prototype.serialize.call(this); Let's extract the values first: let {defaults, _lastDownload} = this; (This should be the first line, even before calling the superclass's serialize method) https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:403: yield* Subscription.prototype.serialize.call(this); Same thing, let's extract _homepage and _lastDownload as the first thing. https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:556: yield* RegularSubscription.prototype.serialize.call(this); Same thing, let's extract all the values of the properties here first before we ever yield anything.
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterClasses.j... lib/filterClasses.js:151: yield "[Filter]"; On 2018/10/09 15:11:03, Manish Jethani wrote: > Let's extract the value of the text property first: > > let {text} = this; > yield "[Filter]"; > yield "text=" + text; Done. https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterClasses.j... lib/filterClasses.js:619: if (this._disabled || this._hitCount || this._lastHit) On 2018/10/09 15:11:03, Manish Jethani wrote: > Similarly, let's extract the values here first: > > let {_disabled, _hitCount, _lastHit} = this; > > The reason for doing this is that the values being serialized should be > consistent. Inconsistent values would be, for example, when hit count is 0 but > last hit is set. Done. https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/09 15:11:03, Manish Jethani wrote: > On 2018/10/06 00:06:32, Jon Sonesen wrote: > > On 2018/10/04 06:10:15, Manish Jethani wrote: > > > On 2018/10/04 05:10:06, Manish Jethani wrote: > > > > On 2018/10/04 04:36:23, Manish Jethani wrote: > > > > > Alright, some more thinking about this. Let's do this in two steps. In > the > > > > first > > > > > step, let's do this: > > > > > > > > > > yield [...subscription.serialize()]; > > > > > > > > Sorry, I mean `yield*` here. > > > > > > I profiled this. It performs slightly better than what we have right now. > > > > > > If we change it to `yield* subscription.serialize()` then it performs > > > significantly better. Again, not surprising, but it's good to verify this. > > > > > > If you'd like to try it out, reload the extension, open the DevTools for the > > > background page, go to the Performance tab and start profiling. Then run > this > > > script, which will open up the Alexa Top 50: > > > > > > #!/bin/bash > > > for domain in $(curl https://www.alexa.com/topsites | sed -nE > > > 's/.*\/siteinfo\/([^"]*).*/\1/p') > > > do > > > open -a 'Google Chrome' "http://$domain" > > > done > > > > > > Stop profiling after approximately one minute, then go to the Bottom-up tab > at > > > the bottom of the window (default layout) and search for "exportData". > > > > So, I had similar concerns. But figured we can hash it out here, and this > > version was more 'to spec' in my mind. > > > > I actually agree that this is a pretty huge change, however doing the change > > which is slightly better performance wise with plans to later make the larger > > change seems like it may be more of a delay than anything since we would still > > need to address the same issue we face here in the future. So, I think it's > > worth it to just take the time to test and properly implement the full change. > > > > > However, I do also realize this change makes the code a bit harder to follow > and > > so we should consider how much that matters here. We are trying to optimize > core > > as much as possible (for good reason) and as long as it makes sense, then I am > > up for it. > > > > Also, if you insist we just hold off for now and use your suggestion I am > > perfectly happy to do so, this was just my two rupees worth of opinion. > > Alright, let's do this properly. > > So the strategy is to take a snapshot of the object right at the beginning of > the serialize function, which makes it just like the current implementation in > terms of serializing a consistent state of the object. > > let {prop1, prop2, prop3} = this; > if (prop1) > yield prop1; > if (prop2) > yield prop2; > if (prop3) > yield prop3; > > This way, if the value of a property changes in between yield statements, it > would still yield the old value. Spent an embarrassing amount of time trying to figure out what to change here but realized this reply is probably meant to describe the yield approach (which is addressed) https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:134: yield "[Subscription]"; On 2018/10/09 15:11:04, Manish Jethani wrote: > Let's extract the values first: > > let {url, type, _title, _fixedTitle, _disabled} = this; > > This gives us a "snapshot" of the object at the time the function is first > called, which means we always serialize a consistent state of the object. Done. https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:285: yield* Subscription.prototype.serialize.call(this); On 2018/10/09 15:11:04, Manish Jethani wrote: > Let's extract the values first: > > let {defaults, _lastDownload} = this; > > (This should be the first line, even before calling the superclass's serialize > method) Done. https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:403: yield* Subscription.prototype.serialize.call(this); On 2018/10/09 15:11:04, Manish Jethani wrote: > Same thing, let's extract _homepage and _lastDownload as the first thing. Done. https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:431: yield new Error( On 2018/10/04 05:30:00, Manish Jethani wrote: > On 2018/10/04 03:37:12, Manish Jethani wrote: > > This would still be a throw. > > I see, you're getting an error from ESLint. I hate ESLint. > > You can just add the following above the `throw` statement: > > // This is just to make ESLint happy. > yield; Inline disable ok? https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:556: yield* RegularSubscription.prototype.serialize.call(this); On 2018/10/09 15:11:04, Manish Jethani wrote: > Same thing, let's extract all the values of the properties here first before we > ever yield anything. Done.
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.j... lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/12 03:50:05, Jon Sonesen wrote: > On 2018/10/09 15:11:03, Manish Jethani wrote: > > On 2018/10/06 00:06:32, Jon Sonesen wrote: > > > On 2018/10/04 06:10:15, Manish Jethani wrote: > > > > On 2018/10/04 05:10:06, Manish Jethani wrote: > > > > > On 2018/10/04 04:36:23, Manish Jethani wrote: > > > > > > Alright, some more thinking about this. Let's do this in two steps. In > > the > > > > > first > > > > > > step, let's do this: > > > > > > > > > > > > yield [...subscription.serialize()]; > > > > > > > > > > Sorry, I mean `yield*` here. > > > > > > > > I profiled this. It performs slightly better than what we have right now. > > > > > > > > If we change it to `yield* subscription.serialize()` then it performs > > > > significantly better. Again, not surprising, but it's good to verify this. > > > > > > > > If you'd like to try it out, reload the extension, open the DevTools for > the > > > > background page, go to the Performance tab and start profiling. Then run > > this > > > > script, which will open up the Alexa Top 50: > > > > > > > > #!/bin/bash > > > > for domain in $(curl https://www.alexa.com/topsites | sed -nE > > > > 's/.*\/siteinfo\/([^"]*).*/\1/p') > > > > do > > > > open -a 'Google Chrome' "http://$domain" > > > > done > > > > > > > > Stop profiling after approximately one minute, then go to the Bottom-up > tab > > at > > > > the bottom of the window (default layout) and search for "exportData". > > > > > > So, I had similar concerns. But figured we can hash it out here, and this > > > version was more 'to spec' in my mind. > > > > > > I actually agree that this is a pretty huge change, however doing the change > > > which is slightly better performance wise with plans to later make the > larger > > > change seems like it may be more of a delay than anything since we would > still > > > need to address the same issue we face here in the future. So, I think it's > > > worth it to just take the time to test and properly implement the full > change. > > > > > > > > However, I do also realize this change makes the code a bit harder to follow > > and > > > so we should consider how much that matters here. We are trying to optimize > > core > > > as much as possible (for good reason) and as long as it makes sense, then I > am > > > up for it. > > > > > > Also, if you insist we just hold off for now and use your suggestion I am > > > perfectly happy to do so, this was just my two rupees worth of opinion. > > > > Alright, let's do this properly. > > > > So the strategy is to take a snapshot of the object right at the beginning of > > the serialize function, which makes it just like the current implementation in > > terms of serializing a consistent state of the object. > > > > let {prop1, prop2, prop3} = this; > > if (prop1) > > yield prop1; > > if (prop2) > > yield prop2; > > if (prop3) > > yield prop3; > > > > This way, if the value of a property changes in between yield statements, it > > would still yield the old value. > > Spent an embarrassing amount of time trying to figure out what to change here > but realized this reply is probably meant to describe the yield approach (which > is addressed) Ack. Yeah, this was just a description. https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionCla... lib/subscriptionClasses.js:431: yield new Error( On 2018/10/12 03:50:06, Jon Sonesen wrote: > On 2018/10/04 05:30:00, Manish Jethani wrote: > > On 2018/10/04 03:37:12, Manish Jethani wrote: > > > This would still be a throw. > > > > I see, you're getting an error from ESLint. I hate ESLint. > > > > You can just add the following above the `throw` statement: > > > > // This is just to make ESLint happy. > > yield; > > Inline disable ok? Yeah, that's probably better. https://codereview.adblockplus.org/29900557/diff/29907580/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29907580/lib/filterClasses.j... lib/filterClasses.js:151: let text = this; This should be: let {text} = this; It's working now purely by accident because `toString` is getting called, which returns the text value. https://codereview.adblockplus.org/29900557/diff/29907580/lib/filterClasses.j... lib/filterClasses.js:624: if (this._disabled) We extracted the values first so we would use them throughout the function. Let's remove the `this.` prefix from these lines. https://codereview.adblockplus.org/29900557/diff/29907580/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29907580/lib/subscriptionCla... lib/subscriptionClasses.js:135: let {url, type, _title, _fixedTitle, _disabled} = this; Let's place this line first before any yield statements, and ideally I would also leave a line after it. https://codereview.adblockplus.org/29900557/diff/29907580/lib/subscriptionCla... lib/subscriptionClasses.js:138: if (this.type) Similarly, we should remove the `this.` prefix here. https://codereview.adblockplus.org/29900557/diff/29907580/lib/subscriptionCla... lib/subscriptionClasses.js:561: let {downloadStatus, lastSuccess, lastCheck, expires, Let's move this line to the top of the function body. We should take a snapshot of the state before we ever yield anything.
https://codereview.adblockplus.org/29900557/diff/29907580/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29907580/lib/subscriptionCla... lib/subscriptionClasses.js:135: let {url, type, _title, _fixedTitle, _disabled} = this; On 2018/10/14 20:05:37, Manish Jethani wrote: > Let's place this line first before any yield statements, and ideally I would > also leave a line after it. Done. https://codereview.adblockplus.org/29900557/diff/29907580/lib/subscriptionCla... lib/subscriptionClasses.js:138: if (this.type) On 2018/10/14 20:05:37, Manish Jethani wrote: > Similarly, we should remove the `this.` prefix here. Done. https://codereview.adblockplus.org/29900557/diff/29907580/lib/subscriptionCla... lib/subscriptionClasses.js:561: let {downloadStatus, lastSuccess, lastCheck, expires, On 2018/10/14 20:05:36, Manish Jethani wrote: > Let's move this line to the top of the function body. We should take a snapshot > of the state before we ever yield anything. Done.
https://codereview.adblockplus.org/29900557/diff/29907580/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29907580/lib/subscriptionCla... lib/subscriptionClasses.js:135: let {url, type, _title, _fixedTitle, _disabled} = this; On 2018/10/21 06:18:23, Jon Sonesen wrote: > On 2018/10/14 20:05:37, Manish Jethani wrote: > > Let's place this line first before any yield statements, and ideally I would > > also leave a line after it. > > Done. Sorry if I wasn't clear, it should be the first line: let {url, type, _title, _fixedTitle, _disabled} = this; yield "[Subscription]"; yield "url=" + url; https://codereview.adblockplus.org/29900557/diff/29907580/lib/subscriptionCla... lib/subscriptionClasses.js:138: if (this.type) On 2018/10/21 06:18:23, Jon Sonesen wrote: > On 2018/10/14 20:05:37, Manish Jethani wrote: > > Similarly, we should remove the `this.` prefix here. > > Done. What about `this.type` and `this._title`?
My bad, apparently I had neglected to commit some local changes prior to upload :/
My bad, apparently I had neglected to commit some local changes prior to upload :/
https://codereview.adblockplus.org/29900557/diff/29918566/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29918566/lib/filterClasses.j... lib/filterClasses.js:146: * Generates serialized filter. I like the previous comment, I think we could just remove the "to an array of strings" part and keep the rest of it. What do you think? https://codereview.adblockplus.org/29900557/diff/29918566/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29918566/lib/filterStorage.j... lib/filterStorage.js:470: yield "[Subscription filters]"; We could actually move this line into serializeFilters so the function could look like this: *serializeFilters() { yield "[Subscription filters]"; for (let filter of this.filters) yield filter.text.replace(/\[/g, "\\["); } And there's no need to check that the list is non-empty above, we're already filtering out empty special subscriptions, and it's harmless to have a "[Subscription filters]" section with no filters (besides because of the async nature of this now we can't really guarantee that the list will be non-empty, even if we first check that it's non-empty). https://codereview.adblockplus.org/29900557/diff/29918566/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29918566/lib/subscriptionCla... lib/subscriptionClasses.js:128: * Serializes the subscription to an array of strings for writing We could remove the "to an array of strings" part from this comment and keep the rest of it.
https://codereview.adblockplus.org/29900557/diff/29918566/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29918566/lib/filterClasses.j... lib/filterClasses.js:151: let {text} = this; Nit: I would leave a line after this just to be consistent with all the other serialize() implementations, but your call.
https://codereview.adblockplus.org/29900557/diff/29918566/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29918566/lib/filterClasses.j... lib/filterClasses.js:146: * Generates serialized filter. On 2018/10/21 21:51:43, Manish Jethani wrote: > I like the previous comment, I think we could just remove the "to an array of > strings" part and keep the rest of it. What do you think? Acknowledged. Yeah I like that better https://codereview.adblockplus.org/29900557/diff/29918566/lib/filterClasses.j... lib/filterClasses.js:151: let {text} = this; On 2018/10/21 21:52:55, Manish Jethani wrote: > Nit: I would leave a line after this just to be consistent with all the other > serialize() implementations, but your call. Yeah good catch https://codereview.adblockplus.org/29900557/diff/29918566/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29918566/lib/filterStorage.j... lib/filterStorage.js:470: yield "[Subscription filters]"; On 2018/10/21 21:51:43, Manish Jethani wrote: > We could actually move this line into serializeFilters so the function could > look like this: > > *serializeFilters() > { > yield "[Subscription filters]"; > > for (let filter of this.filters) > yield filter.text.replace(/\[/g, "\\["); > } > > And there's no need to check that the list is non-empty above, we're already > filtering out empty special subscriptions, and it's harmless to have a > "[Subscription filters]" section with no filters (besides because of the async > nature of this now we can't really guarantee that the list will be non-empty, > even if we first check that it's non-empty). Yeah I will move it. https://codereview.adblockplus.org/29900557/diff/29918566/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29918566/lib/subscriptionCla... lib/subscriptionClasses.js:128: * Serializes the subscription to an array of strings for writing On 2018/10/21 21:51:43, Manish Jethani wrote: > We could remove the "to an array of strings" part from this comment and keep the > rest of it. Acknowledged.
https://codereview.adblockplus.org/29900557/diff/29918566/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29918566/lib/subscriptionCla... lib/subscriptionClasses.js:151: for (let filter of this.filters) I noticed this method goes against our pattern elsewhere. Not sure if it makes sense here since it can have an empty section when writing to disk, but also this will ensure uniform behavior for serialization.
https://codereview.adblockplus.org/29900557/diff/29918566/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29918566/lib/subscriptionCla... lib/subscriptionClasses.js:151: for (let filter of this.filters) On 2018/10/22 19:32:40, Jon Sonesen wrote: > I noticed this method goes against our pattern elsewhere. Not sure if it makes > sense here since it can have an empty section when writing to disk, but also > this will ensure uniform behavior for serialization. Just for the sake of consistency, I wouldn't mind if we added the following line to the start of the function: let {filters} = this; Even though it does nothing, just for consistency. https://codereview.adblockplus.org/29900557/diff/29919558/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29919558/lib/filterStorage.j... lib/filterStorage.js:495: if (subscription.filters.length) We don't really need this check, as I said in my previous comment. We can never guarantee that the list will be non-empty due to the asynchronous nature of this operation. https://codereview.adblockplus.org/29900557/diff/29919558/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29919558/lib/subscriptionCla... lib/subscriptionClasses.js:128: * Serializes the subscription for writing Nit: We could wrap this.
https://codereview.adblockplus.org/29900557/diff/29919558/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29919558/lib/filterStorage.j... lib/filterStorage.js:495: if (subscription.filters.length) On 2018/10/22 20:18:01, Manish Jethani wrote: > We don't really need this check, as I said in my previous comment. We can never > guarantee that the list will be non-empty due to the asynchronous nature of this > operation. My bad, I'll fix it https://codereview.adblockplus.org/29900557/diff/29919558/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29919558/lib/subscriptionCla... lib/subscriptionClasses.js:128: * Serializes the subscription for writing On 2018/10/22 20:18:01, Manish Jethani wrote: > Nit: We could wrap this. Acknowledged. https://codereview.adblockplus.org/29900557/diff/29919558/lib/subscriptionCla... lib/subscriptionClasses.js:160: return [...this.serialize()].join("\n"); This may be unrelated, but I was reading about how spread operators prevent functions from being optimized by the engine. If I recall correctly this method is used in a couple performant reliant functions, no?
https://codereview.adblockplus.org/29900557/diff/29919558/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29919558/lib/subscriptionCla... lib/subscriptionClasses.js:160: return [...this.serialize()].join("\n"); On 2018/10/22 22:59:36, Jon Sonesen wrote: > This may be unrelated, but I was reading about how spread operators prevent > functions from being optimized by the engine. If I recall correctly this method > is used in a couple performant reliant functions, no? I don't know (where is this method called?), but I also don't see a better way of doing it. `Array.from` is even worse.
On 2018/10/22 23:01:30, Manish Jethani wrote: > https://codereview.adblockplus.org/29900557/diff/29919558/lib/subscriptionCla... > File lib/subscriptionClasses.js (right): > > https://codereview.adblockplus.org/29900557/diff/29919558/lib/subscriptionCla... > lib/subscriptionClasses.js:160: return [...this.serialize()].join("\n"); > On 2018/10/22 22:59:36, Jon Sonesen wrote: > > This may be unrelated, but I was reading about how spread operators prevent > > functions from being optimized by the engine. If I recall correctly this > method > > is used in a couple performant reliant functions, no? > > I don't know (where is this method called?), but I also don't see a better way > of doing it. `Array.from` is even worse. I have misspoken it's called in the tests. Nevermind :)
https://codereview.adblockplus.org/29900557/diff/29918566/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29918566/lib/subscriptionCla... lib/subscriptionClasses.js:151: for (let filter of this.filters) On 2018/10/22 20:18:01, Manish Jethani wrote: > On 2018/10/22 19:32:40, Jon Sonesen wrote: > > I noticed this method goes against our pattern elsewhere. Not sure if it makes > > sense here since it can have an empty section when writing to disk, but also > > this will ensure uniform behavior for serialization. > > Just for the sake of consistency, I wouldn't mind if we added the following line > to the start of the function: > > let {filters} = this; > > Even though it does nothing, just for consistency. Done. https://codereview.adblockplus.org/29900557/diff/29919558/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29919558/lib/subscriptionCla... lib/subscriptionClasses.js:160: return [...this.serialize()].join("\n"); On 2018/10/22 23:01:29, Manish Jethani wrote: > On 2018/10/22 22:59:36, Jon Sonesen wrote: > > This may be unrelated, but I was reading about how spread operators prevent > > functions from being optimized by the engine. If I recall correctly this > method > > is used in a couple performant reliant functions, no? > > I don't know (where is this method called?), but I also don't see a better way > of doing it. `Array.from` is even worse. Acknowledged.
Except for a couple of minor nits, LGTM https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... lib/subscriptionClasses.js:128: * Serializes the subscription for writing out on the disk. Nit: "on the disk" ... we could lose the "the" (it's not there in the lib/filterClasses.js version). https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... lib/subscriptionClasses.js:294: if (defaults && defaults.length) Thinking about related changes now, we don't need to check `defaults.length` here, just checking for `defaults` being truthy should be enough.
https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... lib/subscriptionClasses.js:128: * Serializes the subscription for writing out on the disk. On 2018/10/23 03:02:18, Manish Jethani wrote: > Nit: "on the disk" ... we could lose the "the" (it's not there in the > lib/filterClasses.js version). Done. https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... lib/subscriptionClasses.js:294: if (defaults && defaults.length) On 2018/10/23 03:02:18, Manish Jethani wrote: > Thinking about related changes now, we don't need to check `defaults.length` > here, just checking for `defaults` being truthy should be enough. Done.
https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... lib/subscriptionClasses.js:294: if (defaults && defaults.length) On 2018/10/23 03:13:55, Jon Sonesen wrote: > On 2018/10/23 03:02:18, Manish Jethani wrote: > > Thinking about related changes now, we don't need to check `defaults.length` > > here, just checking for `defaults` being truthy should be enough. > > Done. You made the change in the wrong place :)
On 2018/10/23 03:15:59, Manish Jethani wrote: > https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... > File lib/subscriptionClasses.js (right): > > https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... > lib/subscriptionClasses.js:294: if (defaults && defaults.length) > On 2018/10/23 03:13:55, Jon Sonesen wrote: > > On 2018/10/23 03:02:18, Manish Jethani wrote: > > > Thinking about related changes now, we don't need to check `defaults.length` > > > here, just checking for `defaults` being truthy should be enough. > > > > Done. > > You made the change in the wrong place :) Argggh
On 2018/10/23 03:21:38, Jon Sonesen wrote: > On 2018/10/23 03:15:59, Manish Jethani wrote: > > > https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... > > File lib/subscriptionClasses.js (right): > > > > > https://codereview.adblockplus.org/29900557/diff/29919569/lib/subscriptionCla... > > lib/subscriptionClasses.js:294: if (defaults && defaults.length) > > On 2018/10/23 03:13:55, Jon Sonesen wrote: > > > On 2018/10/23 03:02:18, Manish Jethani wrote: > > > > Thinking about related changes now, we don't need to check > `defaults.length` > > > > here, just checking for `defaults` being truthy should be enough. > > > > > > Done. > > > > You made the change in the wrong place :) > > Argggh Should we also wait for snoack on this one?
LGTM On 2018/10/23 03:22:48, Jon Sonesen wrote: > > Should we also wait for snoack on this one? If Sebastian has time he could take a look, though I don't think it's necessary. Note that we have to update the integration notes in the Trac ticket because this will not require changes in the extension (as mentioned in a comment on the issue).
On 2018/10/23 03:26:01, Manish Jethani wrote: > this will not require changes in the extension (as mentioned in a comment on the *now* require
Ack
Committed and pushed: https://hg.adblockplus.org/adblockpluscore/rev/92121171f23e You can close this now.
Thanks for your help! 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
