-
Notifications
You must be signed in to change notification settings - Fork 50
Extension Performance #41
Comments
Yep, this is a hard problem right now because of how the parser is written. I think it arises here: I also think we need to decouple all of the promises in that file so we can test individual functions, which will help with identifying the performance issues/ As far as the chrome thing, I think it has to do with localStorage, and I try and limit the amount of ads we store there to 100, but it still seems too much: We could drop it to 20, because it isn't all that important to store all the ads. Does this sound like something you could help out with? |
Yep, happy to work on this. Thanks for the pointers. I can't promise anything time wise, heading out for vacations mid next week. I'll post here once I'm actively working on it and making progress. |
I've pushed a hotfix here: 69d8324 @jeremybmerrill pointed out that we weren't successfully cleaning up all of the observers. Still, I'd love to rework parser.js into something more sane. I'm going to keep this open as a tracking issue. |
FYI, I'm currently working through a large refactor to make it easier to test the extension, so hold off for now. I'll have something to show tomorrowish. |
Ha, cool. I had just started looking into the slowness of the popup, but I'll hold off. Found this, but am not sure it's our problem. https://stackoverflow.com/questions/26276815/my-chrome-extension-popup-opens-after-a-few-seconds-its-slow-compared-to-other |
this is worth a shot in the meantime: https://stackoverflow.com/a/30164224 |
Yep, that's what I'm gonna try. I guess the idea is that it fools Chrome into leaving the extension running. |
That’d be dumb. We still need to figure out startup performance though.
…On Fri, Mar 16, 2018 at 7:41 AM Jeremy B. Merrill ***@***.***> wrote:
Yep, that's what I'm gonna try. I guess the idea is that it fools Chrome
into leaving the extension running.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#41 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADYRRAGZLCo45YIJcRnNZsZ3sgi7rnyks5te88IgaJpZM4SPIwJ>
.
|
I'm gonna give it a try. Computers do dumb things sometimes. |
It may be worthwhile to get a lighter version of things rendered, then go get ads from teh server, etc. so something happens right away when you click (even if it's just a loading spinner). Excited to see your refactor. |
That’s basically how it works now. I think we’re being blocked on local storage.
… On Mar 16, 2018, at 7:50 AM, Jeremy B. Merrill ***@***.***> wrote:
It may be worthwhile to get a lighter version of things rendered, then go get ads from teh server, etc. so something happens right away when you click (even if it's just a loading spinner). Excited to see your refactor.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
It's consistently taking -- on a fresh start of Chrome -- less than 20ms to read from localStorage, usually 4-8ms. With 20 ads stored in there. I don't think localStorage is the problem. It does take about 150ms to get to the point where it's requesting ads from the server. While that's kind of a lot, impressionistically, the delay is more like 5 seconds. |
Readers have complained that they some times get a Firefox warning that the plugin is blocking the webpage with an stop script option. I've personally noticed that opening the popup in Chrome takes a few seconds after using it for a while.
Things to look into:
Would be nice to solve this with an TTD approach and keeping track of performance over time.
A report about the issue in German
The text was updated successfully, but these errors were encountered: