Avoid building nsDisplayText when text is transparent

RESOLVED FIXED in mozilla34

Status

()

Core
Layout
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

(Blocks: 4 bugs)

Trunk
mozilla34
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 attachments)

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.
Created attachment 8473502 [details] [diff] [review]
patch
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.
That's actually all you need, since visitedness never changes the alpha; we always use the alpha component from the unvisited style.
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.
Attachment #8473502 - Flags: feedback+
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!
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.
Blocks: 881974
Blocks: 960315
Blocks: 924072
Blocks: 810214
Blocks: 1005005
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P2]
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.
Flags: needinfo?(roc)
Created attachment 8476482 [details] [diff] [review]
Don't build display items for text with alph
Attachment #8476482 - Flags: review?(dbaron)
Assignee: nobody → roc
Status: NEW → ASSIGNED
Flags: needinfo?(roc)
Attachment #8476482 - Flags: review?(dbaron) → review+
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."
Maybe, but I didn't...

https://hg.mozilla.org/integration/mozilla-inbound/rev/bbfc46e60d79
https://hg.mozilla.org/mozilla-central/rev/bbfc46e60d79
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.