1352531 - can mozilla::dom::FontFaceSet::FindOrCreateUserFontEntryFromFontFace be faster?

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

In the profile in bug 1329601 comment 13 I noticed that we're spending a substantial amount of time in mozilla::dom::FontFaceSet::FindOrCreateUserFontEntryFromFontFace, most of which is spent in memory allocation and freeing.

Could this be made any faster?

Here's a slice of the profile focusing on this: https://perfht.ml/2oqedI5

Comment 1

[Cameron McCormack (:heycam)]

That seems to indicate that it's Block objects inside gfxSparseBitSet / gfxCharacterMap, which is for compact unicode-range value storage.

There might be ways of improving gfxSparseBitSet, but if are recreating the gfxCharacterMap object for the same FontFace often, we could just cache its value on the FontFace, and update or invalidate it when the descriptors on the FontFace are changed.

Comment 2

[Cameron McCormack (:heycam)]

Attachment: WIP v0.1

Does this help?

Comment 3

[Cameron McCormack (:heycam)]

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

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8853617 [details] [diff] [review]
WIP v0.1

It's hard for me to tell if it helps; I was actually looking at a profile that Ehsan took, and I haven't even gotten the profiler working on my primary machine.

Maybe Ehsan can tell, though?

Comment 5

[Cameron McCormack (:heycam)]

Ehsan, could you try this patch, or give me some instructions so I can see if it helps whatever workload you were profiling?  Thanks!

Comment 6

[(no longer active)]

FWIW this profile came from the test case linked to in bug 1269695 comment 10.  The STR is in the bug.  I'm doing a build to test.

Comment 7

[(no longer active)]

This is a profile from a build before your changes: https://perfht.ml/2pENdoI
We spend 24ms in FindOrCreateUserFontEntryFromFontFace.

This is a profile from a build after your changes: https://perfht.ml/2oXxLGx
The function doesn't show up in the profile any more!

So your patch helps make the function much faster, and if there is no downsides with it we should land it, but it doesn't improve anything in the grand scheme of things...

Comment 8

[(no longer active)]

Comment on attachment 8853617 [details] [diff] [review]
WIP v0.1

BTW super sorry for the long delay here.

Comment 9

[(no longer active)]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #7)
> So your patch helps make the function much faster, and if there is no
> downsides with it we should land it, but it doesn't improve anything in the
> grand scheme of things...

Sorry, I think this came out negatively but I didn't mean it that way!  I meant to say that this test case is really really slow.  But of course it's not this function's fault.  :-)

Comment 10

[Cameron McCormack (:heycam)]

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #9)
> Sorry, I think this came out negatively but I didn't mean it that way!  I
> meant to say that this test case is really really slow.  But of course it's
> not this function's fault.  :-)

No worries, thanks for testing. :-)

Since the patch doesn't need any more work, may as well get it landed.

Comment 11

[Cameron McCormack (:heycam)]

Attachment: patch

This is the same as the WIP patch.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8859860 [details] [diff] [review]
patch

>+  // FIXME(heycam): If we are changing unicode-range, then a FontFace object
>+  // representing this rule must have its mUnicodeRange value invalidated.

Do you need to fix this?  (This is a regression from this bug, right, in that you don't have any code to set mUnicodeRangeDirty to true other than the constructor?)

Comment 13

[Cameron McCormack (:heycam)]

I guess that comment is misplaced, since nsCSSFontFace::SetDesc is only called from nsCSSParser when parsing the rule initially.  nsCSSFontFaceStyleDecl::SetProperty is where we should be noting that we need to handle dynamic updates.

But we do support modifying unicode-range through FontFace::SetUnicodeRange, and we correctly flush the font set in that case.  So I need to set mUnicodeRangeDirty there.

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8859860 [details] [diff] [review]
patch

OK, cancelling review until comment 13 is done.

Comment 15

[Cameron McCormack (:heycam)]

(Oh, I made the change but forgot to upload the patch.  Needinfo to remind myself to upload it when I'm back at the computer the patch is on...)

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Bug 1352531 - Make dom::FontFace cache its gfxCharacterMap instead of rebuilding it every time.

Review commit: https://reviewboard.mozilla.org/r/131944/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131944/

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 8860911 [details]
Bug 1352531 - Make dom::FontFace cache its gfxCharacterMap instead of rebuilding it every time.

https://reviewboard.mozilla.org/r/131944/#review139368

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c4e4f95b499
Make dom::FontFace cache its gfxCharacterMap instead of rebuilding it every time. r=dbaron

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3c4e4f95b499