Open Bug 1694417 Opened 4 years ago Updated 2 years ago

Slow reflow when selecting a large amount of text, on the logged-out view of GitHub's code viewer

Categories

(Core :: Layout: Text and Fonts, defect)

defect

Tracking

()

Performance Impact medium

People

(Reporter: gregtatum, Unassigned)

Details

(Keywords: perf:responsiveness)

Profile: https://share.firefox.dev/3qLQguz
Platform: macOS

Steps to reproduce:

Expected behavior:

  • The browser should behave react quickly to the changes

Actual behavior:

  • The browser becomes very sluggish taking ~7 seconds to respond to a resize event.
Whiteboard: [qf]
Component: Layout → Layout: Text and Fonts

We do have several issues around selection. WebRender made some of them worse, but this one seems to be about layout.

Whiteboard: [qf] → [qf:p2:responsiveness]

From the inverted call stack, I see we spend a lot time in nsINode::ComputeIndexOf(). Set S2 because of very long reflow time per layout triage guideline.

Severity: -- → S2

I think bug 1679645 and co should've improved things.

Nope, this is still slow. Profile with Nightly 95.0a1 (2021-10-09): https://share.firefox.dev/3oQTIG1

Performance Impact: --- → P2
Whiteboard: [qf:p2:responsiveness]

Still slow. Profile with 104.0a1 (2022-06-29): https://share.firefox.dev/3y390ub

This might have gotten better? The STR don't seem anywhere near as bad now. Slightly jankiness but not like >1s hangs like in my comment 5 profile.

https://share.firefox.dev/3W9cNkD is my profile fwiw. Doesn't show selection code during reflow.

Ah, I do actually still see the same substanial jank.

When I tested and saw not-bad-results in comment 6, I was logged in to GitHub -- and this bug seems not-as-bad when you're logged in (because the site is presumably completely different). But if I'm not logged in (fresh profile, or private browsing window), then I still see badness like in the profile in comment 5.

Here's a profile I captured in today's Nightly 2022-12-19, with 1.3-second-long reflows, just like in comment 5:
https://share.firefox.dev/3FJaON3

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

my profile [...] Doesn't show selection code during reflow.

My profile (linked in this comment) shows most of our reflow time spent in this backtrace, inside of ReflowText which calls into selection code via UnionAdditionalOverflow which calls CombineSelectionUnderlineRect:

mozilla::dom::Selection::LookUpSelection(nsIContent*, unsigned int, unsigned int, mozilla::UniquePtr<SelectionDetails, mozilla::DefaultDelete<SelectionDetails> >, mozilla::SelectionType, bool)
nsFrameSelection::LookUpSelection(nsIContent*, int, int, bool) const
nsTextFrame::GetSelectionDetails()
nsTextFrame::CombineSelectionUnderlineRect(nsPresContext*, nsRect&)
nsTextFrame::UnionAdditionalOverflow(nsPresContext*, nsIFrame*, nsTextFrame::PropertyProvider&, nsRect*, bool, bool)
nsTextFrame::ReflowText(nsLineLayout&, int, mozilla::gfx::DrawTarget*, mozilla::ReflowOutput&, nsReflowStatus&)

(In reply to Daniel Holbert [:dholbert] from comment #8)

When I tested and saw not-bad-results in comment 6, I was logged in to GitHub -- and this bug seems not-as-bad when you're logged in (because the site is presumably completely different).

One example of the substantial difference here: the code viewer uses a table in the slow/janky logged-out view, vs. a flexbox-based layout in the just-fine logged-in view. Here's the markup for the code viewer, just showing up through the first line of GitHub's "Code viewer" (with the line-number/code 1 {), in both designs:

Logged out table-table-based design (janky):

<table data-hpc="" class="highlight tab-size js-file-line-container js-code-nav-container js-tagsearch-file" data-tab-size="8" data-paste-markdown-skip="" data-tagsearch-lang="JSON" data-tagsearch-path="cldr-json/cldr-dates-full/main/en/timeZoneNames.json">
  <tbody>
    <tr>
      <td id="L1" class="blob-num js-line-number js-code-nav-line-number js-blob-rnum" data-line-number="1"></td>
      <td id="LC1" class="blob-code blob-code-inner js-file-line">{</td>
    </tr>

Logged in flex-based design (not janky):

<div class="Box-sc-1gh2r6s-0 bjcubc react-code-file-contents" data-tab-size="8" data-paste-markdown-skip="true" style="height: 47200px;" data-hpc="true">
  <div class="react-line-numbers" style="height: 47200px;">
    <div data-line-number="1" class="react-line-number react-code-text" style="padding-right: 16px;">1</div>
    [...all the other line numbers here...]
  </div>
  <div class="react-code-lines" style="height: 47200px;">
    <div data-key="0" class="react-code-text react-code-line-contents">
      <div id="LC1" class="react-file-line html-div" [...]>{</div>
    </div>

(Here, .react-line-numbers and .react-code-lines are each vertical flex container with all of the numbers/lines.)

So: the table-flavored layout here is the problematic one. (I haven't analyzed to the point of knowing why or if the badness is related to tables at all, or if it's just the codepath that GitHub uses with that particular design vs. the other one.)

Summary: Slow reflow when selecting a large amount of text → Slow reflow when selecting a large amount of text, on the logged-out view of GitHub's code viewer

It is related to tables. Selecting a table creates a one selection range per cell (instead of just one for the whole document or so, in the case of flexbox)

Interestingly, I only see janky resizes when my window is sufficiently small (not super small, maybe 1000px or so)

Downgrading to reflect reality, since it's a bit special-casey, and we're not aware of this coming up super often. And to the extent that we understand the issue, it's not trivial to fix (based on how our selection code works for tables; see comment 10).

Severity: S2 → S3

Thanks to jfkthame for the pointer to this bug.

FWIW, the current selection behaviour in tables as described in comment 10 (also when selecting contiguous cells rather than the whole document) is causing perf problems for a11y as well. There are improvements I can make in a11y which will hopefully help address this, but I thought I'd flag this here in case anyone working on this is considering changing the table cell selection behaviour as a possible solution.

You need to log in before you can comment on or make changes to this bug.