1377257 - gfxFont::GetRoundOffsetsToPixels adds malloc/free churn

Status: RESOLVED FIXED

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

Description

[Mats Palmgren (inactive)]

I'm looking at the perf profile for Gmail in bug 1371668 comment 0 and
the following stack (inverted) shows up there:

gfxFont::SplitAndInitTextRun<unsigned char>(DrawTarget *...)
gfxFont::GetRoundOffsetsToPixels(DrawTarget *)
moz_cairo_font_options_destroy
free_impl

The free() seems to be associated with moz_cairo_font_options_destroy
http://searchfox.org/mozilla-central/rev/152c0296f8a10f81185ee88dfb4114ec3882b4c6/gfx/thebes/gfxFont.cpp#846-850


(BTW, it looks like we've investigated this before in bug 1352528.)

Comment 1

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d58822a17ae24c03a01b44dbdb7a09419cbaec7

Comment 2

[Jonathan Kew [:jfkthame]]

Since we took this out of the inner loop, I'm not sure it's significant enough to spend more time on it. That stack shows up in a total of what, one sample of the profile?

(Looking at the innards of cairo_font_options, I think we could actually use a stack-allocated cairo_font_options_t here instead of doing create + destroy, though that feels like a "dirty trick" that violates the intended usage of the cairo API.)

Comment 3

[Mats Palmgren (inactive)]

cairo_font_options_t is opaque though, so we can't use it.
I think we should just extend the API with what we need though:
https://hg.mozilla.org/try/rev/b6fca462dee7fa18100a56c2ccd80f4c992dd37a#l3.21

Comment 4

[Mats Palmgren (inactive)]

*can't use it outside of cairo, that is.

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 1 - Add cairo_scaled_font_get_hint_metrics to avoid malloc/free that is required to use cairo_scaled_font_get_font_options

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 2 - Use cairo_scaled_font_get_hint_metrics instead of cairo_scaled_font_get_font_options to avoid malloc/free associated with cairo_font_options_create/destroy

https://treeherder.mozilla.org/#/jobs?repo=try&revision=befc5126fb08bbb8075d9e72c27dfc16f2a7535e

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8882472 [details] [diff] [review]
part 1 - Add cairo_scaled_font_get_hint_metrics to avoid malloc/free that is required to use cairo_scaled_font_get_font_options

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

LGTM. I'm not sure we care about adding a separate patch file for things like this any more, as we don't try to pull in new cairo releases (do we?); maybe Jeff can comment on that.

::: gfx/cairo/cairo/src/cairo.h
@@ +1497,5 @@
>  cairo_public void
>  cairo_scaled_font_get_font_options (cairo_scaled_font_t		*scaled_font,
>  				    cairo_font_options_t	*options);
>  
> +cairo_public cairo_hint_metrics_t

Might be good to add a "mozilla extension" comment here, as well as at the implementation.

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8882474 [details] [diff] [review]
part 2 - Use cairo_scaled_font_get_hint_metrics instead of cairo_scaled_font_get_font_options to avoid malloc/free associated with cairo_font_options_create/destroy

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

::: gfx/thebes/gfxFont.cpp
@@ +846,3 @@
>    cairo_hint_metrics_t hint_metrics =
> +    cairo_scaled_font_get_hint_metrics(scaled_font);
> +      

nit: stray whitespace

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Is this ready to land, and if it does, is the problem solved with it?

Comment 10

[Jonathan Kew [:jfkthame]]

Yes and yes, AFAICS. My only question was about the mechanics of the cairo patch, as noted in comment 7, but that doesn't need to hold us up, and Jeff may simply not care anyway.

Comment 11

[Jet Villegas (inactive)]

Jonathan: can you land this one for us? Mats is on vacation for another week.

Comment 12

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bca0e256c3cf97e142c17a43b0129a08b90acad0
Bug 1377257 part 1 - Add cairo_scaled_font_get_hint_metrics to avoid malloc/free that is required to use cairo_scaled_font_get_font_options.  r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/53118a4b39b1b09ac796aa406ea8f4b02ce55a0e
Bug 1377257 part 2 - Use cairo_scaled_font_get_hint_metrics instead of cairo_scaled_font_get_font_options to avoid malloc/free associated with cairo_font_options_create/destroy.  r=jfkthame

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/bca0e256c3cf
https://hg.mozilla.org/mozilla-central/rev/53118a4b39b1