1054161 - Avoid building nsDisplayText when text is transparent

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Apparently this will help pdf.js a good bit. There's a text overlay which can contain a lot of very small text blocks (1 character each), each positioned and CSS transformed, all color:transparent. In nsTextFrame::BuildDisplayList, if the color's alpha value is 0, the text is not selected, there are no shadows, and no text decorations, we can skip building the nsDisplayText. Then we will avoid building an nsDisplayTransform wrapping it, and the inactive layer for that.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8473502 [details] [diff] [review]
patch

Review of attachment 8473502 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ +4567,5 @@
>      return;
>    
>    DO_GLOBAL_REFLOW_COUNT_DSP("nsTextFrame");
>  
> +  if (NS_GET_A(GetVisitedDependentColor(eCSSProperty_color)) == 0 &&

Before this NS_GET_A call, insert "NS_GET_A(StyleColor()->mColor) == 0 &&" for more efficiency.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That's actually all you need, since visitedness never changes the alpha; we always use the alpha component from the unvisited style.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8473502 [details] [diff] [review]
patch

Review of attachment 8473502 [details] [diff] [review]:
-----------------------------------------------------------------

I tried the patch (with the suggested edit) and it definitely improved the scrolling of the PDF at https://github.com/mozilla/pdf.js/issues/1045 -- made it smoother and less laggy.

Comment 5

[Nicholas Nethercote [inactive]]

I also tried measuring peak RSS while loading https://github.com/mozilla/pdf.js/issues/2541. Without the patch, it was ~700 MiB. With the patch it was ~530 MiB. Nice!

Comment 6

[Nicholas Nethercote [inactive]]

I did an about:memory diff to see exactly what accounted for the difference in memory usage. It showed up mostly as "heap-unclassified", with a bit of "heap-overhead" as well. So I did some DMD runs and the "heap-unclassified" is all from the FrameLayerBuilder -- see bug 1054321.

Comment 7

[Nicholas Nethercote [inactive]]

roc: is anything else needed to land this? You can see from the blocking bugs that there are lots of cases where this will help pdf.js performance greatly.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Don't build display items for text with alph

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8476482 [details] [diff] [review]
Don't build display items for text with alph

Review of attachment 8476482 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsTextFrame.cpp
@@ +4580,5 @@
>    
>    DO_GLOBAL_REFLOW_COUNT_DSP("nsTextFrame");
>  
> +  if (NS_GET_A(StyleColor()->mColor) == 0 &&
> +      !IsSVGText() && !IsSelected() && !StyleText()->HasTextShadow()) {

Would a short comment explaining the motivation be useful? E.g. "pdf.js generates lots of transparent text divs, so optimize that case."

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe, but I didn't...

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfc46e60d79

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bbfc46e60d79