1480801 - Merge consecutive FillGlyph draw commands with compatible parameters

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, enhancement, P3)

Description

[Ryan Hunt [:rhunt]]

From local testing on my alexa top 50 simulation [1]. This can result in a ~20% average improvement to command list size.

[1] https://github.com/eqrion/moz-simulation-script

Comment 1

[Ryan Hunt [:rhunt]]

Attachment: merge-glyphs-rollup.patch

Here's a work-in-progress patch set that I've been working on.

It should mostly work, the last thing remaining is to compare the Patterns when merging FillGlyph commands. Unfortunately it looks like that will need to be a deep comparison to be accurate.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: Bug 1480801 - Add bounds hint to FillGlyphs and StrokeGlyphs.

Comment 3

[Ryan Hunt [:rhunt]]

Attachment: Bug 1480801 - Update text painting to use dirty rect as bounds hint.

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: Bug 1480801 - Merge consecutive FillGlyphs commands with compatible parameters.

Comment 5

[Ryan Hunt [:rhunt]]

Attachment: Bug 1480801 - Add implementations of StrokeGlyphs to DrawTargetDual and DrawTargetTiled.

The default implementation falls back to stroking a path. It'd be better
to just pass the draw command on to destination target(s), in the case
they implement a faster version.

Comment 6

[Ryan Hunt [:rhunt]]

I've polished the patches up a little bit more. The basic idea here is two parts.

1. Stop producing a clip draw command around each text display item
2. Merge FillGlyph commands with compatible parameters

Without (1), (2) is of limited usefulness.

The tricky part is (1). To my knowledge, that clip was added to allow DrawTargetTiled
to cull out FillGlyph commands to only the tiles that need the command, and not for
correctness. So I wrote a commit that instead adds a 'bounds hint' parameter to FillGlyphs
to do the same thing.

Comment 7

[Bas Schouten (:bas.schouten)]

How does this affect performance on D2D?

Comment 8

[Ryan Hunt [:rhunt]]

(In reply to Bas Schouten (:bas.schouten) from comment #7)
> How does this affect performance on D2D?

I don't see any significant difference from a talos run [1].

[1] https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=130bb81b5f4f&newProject=try&newRevision=7ee15f1064f139c0efbccdf4846a7243c152e60c&framework=1&filter=windows10

Comment 9

[Lee Salzman [:lsalzman]]

Comment on attachment 9000154 [details]
Bug 1480801 - Update text painting to use dirty rect as bounds hint.

Lee Salzman [:lsalzman] has approved the revision.

Comment 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 9000153 [details]
Bug 1480801 - Add bounds hint to FillGlyphs and StrokeGlyphs.

Bas Schouten (:bas.schouten) has approved the revision.

Comment 11

[Bas Schouten (:bas.schouten)]

Comment on attachment 9000155 [details]
Bug 1480801 - Merge consecutive FillGlyphs commands with compatible parameters.

Bas Schouten (:bas.schouten) has approved the revision.

Comment 12

[Bas Schouten (:bas.schouten)]

Comment on attachment 9000156 [details]
Bug 1480801 - Add implementations of StrokeGlyphs to DrawTargetDual and DrawTargetTiled.

Bas Schouten (:bas.schouten) has approved the revision.

Comment 13

[Bas Schouten (:bas.schouten)]

Comment on attachment 9000153 [details]
Bug 1480801 - Add bounds hint to FillGlyphs and StrokeGlyphs.

Bas Schouten (:bas.schouten) has been removed from the revision.

Comment 14

[Mayank Bansal]

Bas, is this bug and the patches still relevant?

Comment 15

[Bas Schouten (:bas.schouten)]

I don't think it is. Closing.