Closed Bug 1420628 Opened 2 years ago Closed Last year

gfxContext + TextDrawTarget creation and destruction slows down nsDisplayText::CreateWebRenderCommands

Categories

(Core :: Graphics: WebRender, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox59 --- affected
firefox65 --- fixed

People

(Reporter: mstange, Assigned: mattwoodrow)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [wr-reserve])

Attachments

(1 file)

Here's a profile I got from scrolling https://w3c.github.io/ServiceWorker/ with a profiler interval of 0.2ms on macOS:

https://perfht.ml/2k2CpCJ

Around 20% of the nsDisplayText::CreateWebRenderCommands is taken up by memory allocation + deallocation of gfxContext and TextDrawTarget. It would be nice to avoid that cost.

For example, we could have a gfxContext wrapping a TextDrawTarget stored on the StackingContextHelper, and give the TextDrawTarget a way to reset itself. Then every nsDisplayText::CreateWebRenderCommands invocation could just get that context from the StackingContextHelper, and the StackingContextHelper would reset the TextDrawTarget just before handing out a pointer to that gfxContext.
Whiteboard: [wr-mvp] [triage]
Priority: -- → P3
Whiteboard: [wr-mvp] [triage] → [wr-mvp] [triage][wr-reserve-candidate]
Whiteboard: [wr-mvp] [triage][wr-reserve-candidate] → [wr-reserve]
I don't think this should be a blocker on the first release.
Blocks: stage-wr-next
No longer blocks: stage-wr-nightly
One way to help this would be to eliminate the need for a gfxContext in our text drawing code.

One of the pieces to doing that is to fix the use of an implicit gfxPattern and make it explicit. To make this work we'll need to adjust the gfxPattern matrix so that the context scaling that happens during text drawing doesn't affect the pattern matrix. Today this is all happening in implicitly on the gfxContext.
I have a patch that implements Markus' idea for this (written before I saw this bug), and it makes a fairly significant difference.

I do agree that getting rid of the gfxContext dependency would be nice, but I don't think it's something we want on our critical path right now, and it's the simpler of the two objects to retain.
Assignee: nobody → matt.woodrow
I thought you might tackle this. Do you have an idea of how much difference it makes?
On the testcase we were discussing yesterday (reddit comments page, with an inline youtube video playing at the top), the patch queue in differential saves about 20% (2.5ms -> 2.0ms). This was the most significant chunk of that.

I have a displaylist_mutate testcase that includes text, which may be less realistic, but easier to measure. I'll test it out.
Very nice.
It's a bit hard to measure dispaylist_mutate scores, since we're so scene building bound.

Using displaylist_mutate with text [1] (lots of opacity items, containing text and background color), this patch alone removes ~5ms per frame (45ms -> 40ms) from the RenderLayers section.


[1] https://mattwoodrow.github.io/dl-test/dl-test.html?layer=flattened
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5c8fe43b6ee
Avoid allocations for each text display item. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/f5c8fe43b6ee
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.