Performance issue on PRs with lots of changes (3rd issue)

[fregante]

Follows

• Performance issue on PRs with lots of changes (2nd issue) #7316

If after v24.4.9 you still see performance issues, please "like" this issue.

Please do not leave "me too" comments

[ewjoachim]

(Just a small comment: I thought I was still affected, but then realized that firefox had not updated the lib. I was still on 24.4.1, I had to manually check version on about:addons, and manually request an upgrade to test the 24.4.9 which worked for me[apparently not]. Please triple-check your version before adding a 👍 )

[rbclark]

In my case I was able to use the 'identify feature' function to track the culprit down to quick-mention. In order to trigger it I have to switch between the conversation and files tabs first. I've verified that I am on 24.4.9 and was still experiencing the issue until I disabled the aforementioned feature.

[kaste]

I used "identify feature" and had to disable quick-mention and locked-issue. I'm on 24.4.9 using Brave.

[jryans]

quick-mention seems to be the culprit for me as well. Here's a Firefox profile of the ~1 min hang I see if that feature is enabled when trying to view a large PR.

The profile suggests some code calls Element.replaceWith, which then notifies some mutation observes, and then spends a very long time invalidating styles. I don't entirely understand which part of quick-mention is connected here... perhaps the selector this feature passes to observe doesn't perform well in Firefox...?

[fregante]

My guesses are:

• some code hits some heavily-unoptimized part of both Firefox and Safari (Chrome performs much better in general)
• our "page unload" handler fires too late because GitHub no longer has a good event for that, so existing selectors from the old page fire on the new DOM just before the new "page load", causing havoc somehow

There's an issue for the latter. I will try to look into that specifically and hope it magically solves at least half the issue.

[jryans]

Ah I see, after just reading the context in older linked issues, I guess the root cause is features like quick-mention shouldn't even be activating on the PR files view... They only trigger when first viewing the PR conversation, then trying to go to PR files in the same tab. The page change is not noticed / noticed too late by extension features, and thus things like quick-mention remain active during the transition from PR conversation to PR files, even though that is not intentional.

[jryans]

I filed a Firefox bug about this situation as well, which links back to the discussion here. Even though this add-on is involved in the situation, there does seem to be some kind of core Firefox performance issue that is being triggered by this add-on's behaviour, so it's possible a future Firefox version may perform better here.

I would imagine for the short term though, changing the add-on behaviour as suggested above is the quickest route to a fix.

[fregante]

Thank you for filing the bug. Hopefully a mozillian can look into what the browser is spending cycles on, because I can't decipher its dev tools

[jryans]

Mozillians have made some changes to Firefox which have now landed in the latest nightly version (2024-05-18), and I can confirm performance is much improved. 😄 For the PR I've been testing with, there's a 4 - 5x performance improvement with this Firefox change.

The add-on features mentioned here (like quick-mention) still do cause a perceptible delay / jank on large PRs though, so I think it would still be ideal to improve this add-on where possible (perhaps by improving the previously mentioned page navigation detection).

[fregante]

Happy to hear that!

Is there an issue with the selector? Can that be improved too?

The AJAX I think just makes the issue more visible, but if a selector performs badly it will continue to do so on every DOM change.

[jryans]

Is there an issue with the selector? Can that be improved too?

I wasn't entirely sure myself. After chatting with Mozillians about it, they said that anchoring :has() to an element (or to all elements) slows down all DOM mutations inside that subtree (with more performance impact the more complex the selector inside :has() is). So stuff like body:has() or :root:has() or *:has() are recipes for bad performance.

For example, the quick-mention feature uses body:has(). It would likely be better to use a more specific anchor than body here. It's perhaps even better to avoid using has() if a simpler selector would suffice.

[fregante]

I made some changes as suggested by @jryans in the upcoming 24.6.2 version.

I'm closing this issue and I'll reopen another one in a couple of days to check how we're doing.

As always, before reporting your issues, ensure you're on the latest (24.6.2 as of today)