Markus Stange [:mstange] Reporter
	Description • 7 years ago

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.

	Aria Beingessner [:Gankra]
	Comment 1 • 6 years ago

I don't think this should be a blocker on the first release.

	Jeff Muizelaar [:jrmuizel]
	Comment 2 • 6 years ago

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.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 3 • 6 years ago

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.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 4 • 6 years ago

MozReview-Commit-ID: HoRHQVEdJeT

	Jeff Muizelaar [:jrmuizel]
	Comment 5 • 6 years ago

I thought you might tackle this. Do you have an idea of how much difference it makes?

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 6 • 6 years ago

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.

	Jeff Muizelaar [:jrmuizel]
	Comment 7 • 6 years ago

Very nice.

	Matt Woodrow (:mattwoodrow) Assignee
	Comment 8 • 6 years ago

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

	Pulsebot
	Comment 9 • 6 years ago

Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5c8fe43b6ee
Avoid allocations for each text display item. r=jrmuizel

	Natalia Csoregi [:nataliaCs]
	Comment 10 • 6 years ago
bugherder

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