facebook-political-ads icon indicating copy to clipboard operation
facebook-political-ads copied to clipboard

Extension Performance

Open tpreusse opened this issue 7 years ago • 12 comments

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

tpreusse avatar Feb 22 '18 11:02 tpreusse

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?

thejefflarson avatar Feb 22 '18 21:02 thejefflarson

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.

tpreusse avatar Feb 22 '18 23:02 tpreusse

I've pushed a hotfix here: https://github.com/propublica/facebook-political-ads/commit/69d8324f73a6de53403d0dfa8b06d3fc6b57452e

@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 avatar Mar 06 '18 12:03 thejefflarson

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.

thejefflarson avatar Mar 15 '18 22:03 thejefflarson

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

jeremybmerrill avatar Mar 15 '18 22:03 jeremybmerrill

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

thejefflarson avatar Mar 15 '18 22:03 thejefflarson

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

jeremybmerrill avatar Mar 16 '18 14:03 jeremybmerrill

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 [email protected] 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 https://github.com/propublica/facebook-political-ads/issues/41#issuecomment-373734091, or mute the thread https://github.com/notifications/unsubscribe-auth/AADYRRAGZLCo45YIJcRnNZsZ3sgi7rnyks5te88IgaJpZM4SPIwJ .

thejefflarson avatar Mar 16 '18 14:03 thejefflarson

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

jeremybmerrill avatar Mar 16 '18 14:03 jeremybmerrill

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.

jeremybmerrill avatar Mar 16 '18 14:03 jeremybmerrill

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 [email protected] 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.

thejefflarson avatar Mar 16 '18 15:03 thejefflarson

        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.

jeremybmerrill avatar Mar 16 '18 17:03 jeremybmerrill