Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(363)

Issue 29900557: Issue 7016 - Convert serialization functions into generators

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 2 days ago by Jon Sonesen
Modified:
1 day, 20 hours ago
Visibility:
Public.

Description

Issue 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: 5

Patch Set 4 : Address PS3 Comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+87 lines, -86 lines) Patch
M lib/filterClasses.js View 1 2 3 4 chunks +19 lines, -16 lines 0 comments Download
M lib/filterStorage.js View 1 chunk +4 lines, -12 lines 0 comments Download
M lib/subscriptionClasses.js View 1 2 3 5 chunks +62 lines, -54 lines 0 comments Download
M test/filterClasses.js View 1 2 3 1 chunk +1 line, -2 lines 0 comments Download
M test/subscriptionClasses.js View 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 13
Jon Sonesen
2 weeks, 2 days ago (2018-10-03 23:48:03 UTC) #1
Manish Jethani
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js#oldcode467 lib/filterStorage.js:467: yield ""; I think we still need the blank ...
2 weeks, 1 day ago (2018-10-04 03:37:13 UTC) #2
Manish Jethani
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js#oldcode472 lib/filterStorage.js:472: buf.push("", "[Subscription filters]"); We would have to yield a ...
2 weeks, 1 day ago (2018-10-04 04:09:00 UTC) #3
Manish Jethani
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js#newcode468 lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/04 04:08:59, Manish Jethani wrote: > ...
2 weeks, 1 day ago (2018-10-04 04:17:06 UTC) #4
Manish Jethani
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js#newcode468 lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/04 04:17:06, Manish Jethani wrote: > ...
2 weeks, 1 day ago (2018-10-04 04:36:23 UTC) #5
Manish Jethani
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js#oldcode467 lib/filterStorage.js:467: yield ""; On 2018/10/04 03:37:12, Manish Jethani wrote: > ...
2 weeks, 1 day ago (2018-10-04 04:50:11 UTC) #6
Manish Jethani
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js#newcode468 lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/04 04:36:23, Manish Jethani wrote: > ...
2 weeks, 1 day ago (2018-10-04 05:10:07 UTC) #7
Manish Jethani
https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionClasses.js File lib/subscriptionClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/subscriptionClasses.js#newcode431 lib/subscriptionClasses.js:431: yield new Error( On 2018/10/04 03:37:12, Manish Jethani wrote: ...
2 weeks, 1 day ago (2018-10-04 05:30:00 UTC) #8
Manish Jethani
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js#newcode468 lib/filterStorage.js:468: if (subscription.filters.length) On 2018/10/04 05:10:06, Manish Jethani wrote: > ...
2 weeks, 1 day ago (2018-10-04 06:10:15 UTC) #9
Jon Sonesen
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js File lib/filterStorage.js (left): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterStorage.js#oldcode467 lib/filterStorage.js:467: yield ""; On 2018/10/04 04:50:11, Manish Jethani wrote: > ...
1 week, 6 days ago (2018-10-06 00:06:34 UTC) #10
Manish Jethani
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterClasses.js#newcode151 lib/filterClasses.js:151: yield "[Filter]"; Let's extract the value of the text ...
1 week, 3 days ago (2018-10-09 15:11:04 UTC) #11
Jon Sonesen
https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29900557/diff/29900565/lib/filterClasses.js#newcode151 lib/filterClasses.js:151: yield "[Filter]"; On 2018/10/09 15:11:03, Manish Jethani wrote: > ...
1 week ago (2018-10-12 03:50:06 UTC) #12
Manish Jethani
5 days, 3 hours ago (2018-10-14 20:05:37 UTC) #13
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5