Skip to content
This repository has been archived by the owner on Mar 3, 2022. It is now read-only.

Extension Performance #41

Open
tpreusse opened this issue Feb 22, 2018 · 12 comments
Open

Extension Performance #41

tpreusse opened this issue Feb 22, 2018 · 12 comments
Assignees

Comments

@tpreusse
Copy link
Contributor

tpreusse commented Feb 22, 2018

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:

  • large local data set leading to slow initialisation
  • loading too many languages
  • too aggressive facebook.com DOM access

Would be nice to solve this with an TTD approach and keeping track of performance over time.

A report about the issue in German

@thejefflarson
Copy link
Contributor

Yep, this is a hard problem right now because of how the parser is written. I think it arises here:
https://github.com/propublica/facebook-political-ads/blob/master/extension/src/parser.js#L221
Which was my way of figuring out how to grab the ad endpoint from facebook's popup. I'd love it if you could take a little time to see if we can scope that selector better so as not to pick up so many changes.

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:
https://github.com/propublica/facebook-political-ads/blob/master/extension/src/utils.js#L50

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?

@tpreusse
Copy link
Contributor Author

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.

@thejefflarson
Copy link
Contributor

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.

@thejefflarson thejefflarson self-assigned this Mar 15, 2018
@thejefflarson
Copy link
Contributor

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.

@jeremybmerrill
Copy link
Contributor

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

@thejefflarson
Copy link
Contributor

this is worth a shot in the meantime: https://stackoverflow.com/a/30164224

@jeremybmerrill
Copy link
Contributor

Yep, that's what I'm gonna try. I guess the idea is that it fools Chrome into leaving the extension running.

@thejefflarson
Copy link
Contributor

thejefflarson commented Mar 16, 2018 via email

@jeremybmerrill
Copy link
Contributor

I'm gonna give it a try. Computers do dumb things sometimes.

@jeremybmerrill
Copy link
Contributor

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.

@thejefflarson
Copy link
Contributor

thejefflarson commented Mar 16, 2018 via email

@jeremybmerrill
Copy link
Contributor

        var ls_start_time = new Date();
        persistedState = deserialize(localStorage.getItem(key));
        finalInitialState = merge(initialState, persistedState);
        console.log("loaded state from localStorage in ", new Date() - ls_start_time);

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants