4389: Using lots of <style> elements that get incrementally added to a document can cause performance problems

bzbarsky
chipx86
chipx86
4394

What version are you running?

2.5.4

What's the URL of the page containing the problem?

The diff view for a changeset that touches many files.

What steps will reproduce the problem?

  1. Load the diff in Firefox
  2. Observe that it chews a lot of CPU

Please provide any additional information below.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1213122#c13 for analysis.

#1 glob

the firefox developers are working on improving the situation, however they said the situation could be improved by some changes to review board. after some discussion with both review board and firefox developers, it appears that if the styles were inlined onto the element instead of in a <style> block it would result in a better experience.

chipx86
#4 chipx86

I'll give some details as to why we have the multiple <style> blocks here.

In the diff viewer, we have some intelligence around how the diff tables size the columns, in order to have consistent, sane column widths when wrapping long lines in diffs. We have a lot of logic and CSS designed to get diffs to wrap sanely, and to not overflow certain content passed the bounds of a window. We introduced logic in commit 49c378b2 in order to address this. We then had issues with the display of filenames/revisions with long lines, and needed to make additional changes in commit bcc3333c.

So what we do in these changes is, on initial render and resize, we figure out what the widths need to look like, and we set min/max widths for the columns and content. This means a <style> for each, because they're not always necessarily going to be the same min/max widths.

However, they might be, and might be often enough in many cases, where it'd probably be worth reusing <style>s. The way I see that happening is having the current code responsible for these calculations and for the <style> (js/diffviewer/views/diffReviewableView.js) emit a signal with the new widths, instead of setting the CSS directly, and then having the consumer manage a collection of <styles> in some form. Maybe a class per unique values, with the DiffReviewableViews having their classes set accordingly?

This is all a guess. I don't know what's causing Firefox to be slow where other browsers aren't (and I'm fairly sure this didn't used to be slow in Firefox), so I couldn't say whether managing a minimal set of <style>s and assigning classes will solve anything or not.

By the way, it'd be very nice to have discussion here, or at least important bits relayed here. Our policy going forward is that the Mozilla bug tracker will be treated by us as an internal-only tracker. We won't be participating in or working with Mozilla bug reports for Review Board.

#5 glob

here's the change we're using at mozilla, which addresses this issue.
https://hg.mozilla.org/webtools/reviewboard/rev/c4124cae0efd

christian has indicated this isn't his preferred solution, so i won't attach this as a patch for review.

chipx86
#6 chipx86

I know this has been a while, but this hasn't actually dropped off my radar (though I haven't claimed this bug yet -- fixing it). I've finally had some time to do some deep diving into the performance issues, and some really good pathological diffs that help trigger it. 2.5.10 will have a number of improvements here, and hopefully will address this. The style tags are gone in the version I have in development (though I'm also trying to get the same smart resizing logic we had before).

Worth noting that setting the style individually for all the pre elements wrecks performance for large changes, so we can't go that route, but there's some tricks I've come up with that should get us the same result with less work for the browser.

  • -New
    +Confirmed
  • +Release-2.5.x
  • -Priority:Medium
    +Component:DiffViewer
    +Performance
    +Priority:High
  • +chipx86
chipx86
#7 chipx86

I believe I've solved this. There's a handful of changes going into 2.5.11 that address some performance bottlenecks that happen during initial render and resizing, and redoes the styling logic to no longer inject a <style> or iterate through all rows.

chipx86
#8 chipx86

Guess I never resolved this. It's going into 2.5.11 tomorrow. There are a lot of changes that went into performance improvments for the diff viewer, but this particular one is fixed in c44a8806a4d6a95c491a757d60774b8edf380e58.

  • -Confirmed
    +Fixed