Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Very slow performance on page with Facebook Container extension enabled #601

Closed
dremin opened this issue Feb 18, 2020 · 11 comments · Fixed by #736
Closed

Very slow performance on page with Facebook Container extension enabled #601

dremin opened this issue Feb 18, 2020 · 11 comments · Fixed by #736
Labels
enhancement New feature or request needs-priority This issue needs prioritization

Comments

@dremin
Copy link

dremin commented Feb 18, 2020

  • Facebook Container Version: 2.0.3
  • Operating System + Version: macOS 10.15.3
  • Firefox Version: 75.0a1 (2020-02-18) (64-bit)
  • Other installed Add-ons + Version + Enabled/Disabled-Status: No other extensions enabled with this profile

When using the accordions on the page https://developer.mypurecloud.com/api/rest/v2/conversations/index.html, the web page freezes with Facebook Container enabled. With the extension disabled, the page is relatively smooth.

Actual behavior

Web page is very slow. Profile: https://perfht.ml/38HAxnk

Expected behavior

Expected web page to be smooth, which it is with the Facebook Container extension disabled.

Steps to reproduce

  1. Enable Facebook Container extension
  2. Visit https://developer.mypurecloud.com/api/rest/v2/conversations/index.html
  3. Open one of the accordion items (example in the profile is the GET /api/v2/conversations/messages/{conversationId}/messages/{messageId} accordion item)
  4. Notice web page freezes for a few seconds before the item opens.
  5. Repeat with Facebook Container disabled, and notice performance is smooth.
@jakub-g
Copy link

jakub-g commented Feb 24, 2020

I just noticed a super slow performance on Github pull request page with the addon enabled.

  1. Enable FB container addon.
  2. Open some GH PR page which has >50 files
  3. Click "Viewed" on one of those files
    ⚠️ it takes about 2 seconds for the DOM update to happen
  4. Disable FB container addon and refresh the PR page.
  5. Click "Viewed" on one of those files
    🆗 it's near-instant.

@maxxcrawford maxxcrawford added needs-priority This issue needs prioritization 🐛 bug Something isn't working enhancement New feature or request and removed needs-priority This issue needs prioritization 🐛 bug Something isn't working labels May 13, 2020
@mrwensveen
Copy link
Contributor

I suspect this has to do with memory usage. I notice this behavior on pages that use 150+ MB (as shown in about:performance). DOM updates with Facebook Container enabled take much longer.

It's not so much the amount of DOM nodes, however. I tested with a page that uses a lot of data: URL's for images, and with the same page where those URL's are shorter (using URL.createObjectURL). The amount of DOM nodes is the same, but the memory consumption much lower. Facebook Container makes the inoptimized version almost unusable, while the low-memory version works fine with FBC enabled.

@mrwensveen
Copy link
Contributor

While playing with the source in the add-on debugger I discovered that detectFacebookOnPage is the main culprit, with the patternDetection calls taking up a lot of time. The function is called every time a user clicks on the page (and then set up to periodically run again after a timeout).

What is the thought behind doing this on every click? Maybe there's a way to listen for changes to the DOM and only running the patternDetection function against the changed parts?
Another option would be to optimize patternDetection itself, but I don't know if there's an alternative to document.querySelectAll.

@mrwensveen
Copy link
Contributor

Hello again. I've created a patch that uses MutationObserver to detect changes in the DOM and runs the pattern detection on the changed nodes instead of the entire document (contentScriptInit). I've also removed the contentScriptInit call from the click action, because the mutation observer already does everything.

I think my code might need some polishing. Will you accept a PR?

@maxxcrawford
Copy link
Collaborator

While playing with the source in the add-on debugger I discovered that detectFacebookOnPage is the main culprit, with the patternDetection calls taking up a lot of time. The function is called every time a user clicks on the page (and then set up to periodically run again after a timeout).

What is the thought behind doing this on every click? Maybe there's a way to listen for changes to the DOM and only running the patternDetection function against the changed parts?
Another option would be to optimize patternDetection itself, but I don't know if there's an alternative to document.querySelectAll.

The inital goal for the content script on non-FB pages was to badge Facebook share/login buttons across the web. Often times, these buttons would show up after a user-click (Example: Clicking the "login" button in a nav triggers a modal with a "Login with Facebook" button). The constant checking was to catch these.

Using MutationObserver sounds like it would absolutely be a performance improvement over contentScriptInit and user actions. Please open a PR and we can look. One caveat is that we most likely need our internal QA team to test a release with that accepted, so the timeline for acceptance may not be instanteous.

Thanks for digging in on this @mrwensveen!

@maxxcrawford
Copy link
Collaborator

Oh - One thing I should add. We have a draft PR to add a settings panel to this add-on. (#721) One of the optional settings we're adding is turning off the badge detection entirely. Because that setting is optional, I beleive fixing the memory consumption is still the best/right course of action.

@barryvan
Copy link

We've run into this issue as well with an application we're building -- with Facebook Container enabled, performance falls completely off a cliff. Disabling the addon fixes things. My investigations match @mrwensveen's observations -- it seems as though there's a querySelectorAll being run inside the pattern detection after every click anywhere on the page. As you can see in this screenshot, it makes things very slow...

image

Admittedly the context that this is happening in has a huge number of DOM nodes, but it makes the entire thing unusable.

Is there anything I can do to help getting this PR through and an update to the addon released?

With regard to #721, is it feasible for a site to somehow opt-out of this badge detection, on the basis that it won't be necessary? I don't believe this would undermine the utility of the addon itself, although I admittedly am not an expert.

@KL13NT
Copy link

KL13NT commented May 15, 2021

This is still an issue. Heavy websites have very degraded performance when this extension is enabled. Why doesn't the extension inspect request links instead of looking at the DOM (which is costly)? It'd make more sense to block an outgoing request to Facebook tracking instead of inspecting DOM elements IMHO.

On this page specifically with the extension enabled, trying to change filters adds a huge overhead the average that drops the average FPS to less than 10. In contrast, with the extension disabled the average FPS remains above 30 FPS. This gets worse in interactive websites.

I'm currently debugging on Firefox 88.0.1 (64-bit) latest.

Proposed Solution

I recommend using background scripts to inspect requests instead of looking at the DOM, which would make up for a huge performance improvement. This could affect the user experience since it'd completely disable the "Facebook Container" badge on links and buttons, but is fixable. Instead of actively scanning the DOM for changes, you could use the mousemove event to detect hover over links and buttons and then apply existing UI logic such as adding the badge.

@NPatch
Copy link

NPatch commented May 26, 2022

Still an issue. I can't even open up a message on firefox anymore because it goes on an endless loading binge. Loading posts, loading the chats list and so on.

@mrwensveen
Copy link
Contributor

It seems my PR has some merge conflicts now (it hadn't when I created it). I'll try to take a look at it. It may help to get this through.

@mrwensveen
Copy link
Contributor

I fixed a few merge conflicts. Should be okay now. Please merge my PR, @maxxcrawford 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-priority This issue needs prioritization
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants