gfxFont::GetRoundOffsetsToPixels adds malloc/free churn

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: jfkthame, NeedInfo)

Tracking

({perf})

Trunk
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(2 attachments)

Reporter

Description

2 years ago
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.)
Assignee

Comment 2

2 years ago
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.)
Reporter

Comment 3

2 years ago
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
Reporter

Comment 4

2 years ago
*can't use it outside of cairo, that is.
Assignee

Comment 7

2 years ago
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.
Attachment #8882472 - Flags: review?(jfkthame)
Attachment #8882472 - Flags: review+
Attachment #8882472 - Flags: feedback?(jmuizelaar)
Assignee

Comment 8

2 years ago
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
Attachment #8882474 - Flags: review?(jfkthame) → review+
Whiteboard: [qf] → [qf:p1]
Is this ready to land, and if it does, is the problem solved with it?
Flags: needinfo?(mats)
Flags: needinfo?(jfkthame)
Assignee

Comment 10

2 years ago
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.
Flags: needinfo?(jfkthame)
Flags: needinfo?(milan)
Jonathan: can you land this one for us? Mats is on vacation for another week.
Assignee: mats → jfkthame
Assignee

Comment 12

2 years ago
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

Updated

2 years ago
Depends on: 1390317

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bca0e256c3cf
https://hg.mozilla.org/mozilla-central/rev/53118a4b39b1
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Reporter

Updated

2 years ago
Flags: needinfo?(mats)
You need to log in before you can comment on or make changes to this bug.