1381434 - use HTTP OMT data delivery while loading fontface

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

FontFaceSet uses nsStreamLoader to load font file. We should be able to utilize off-main-thread ODA delivery to reduce main thread usage.

https://searchfox.org/mozilla-central/rev/01d27fdd3946f7210da91b18fcccca01d7324fe2/layout/style/FontFaceSet.cpp#687

Comment 1

[Jet Villegas (inactive)]

(In reply to (PTO 9/21 - 9/24) Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #0)
> FontFaceSet uses nsStreamLoader to load font file. We should be able to
> utilize off-main-thread ODA delivery to reduce main thread usage.
> 
> https://searchfox.org/mozilla-central/rev/
> 01d27fdd3946f7210da91b18fcccca01d7324fe2/layout/style/FontFaceSet.cpp#687

Sounds like a great idea. S-C: are you able to put this together?

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: Bug 1381434 - off-main-thread loading web font

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

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Looks like nsStreamLoader is simple enough to do OMT already. Let's see how this quick patch goes.

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8913501 [details]
Bug 1381434 - off-main-thread loading web font

https://reviewboard.mozilla.org/r/184852/#review190310

::: layout/style/nsFontFaceLoader.cpp:302
(Diff revision 1)
> +nsFontFaceLoader::OnStartRequest(nsIRequest *aRequest,
> +                                 nsISupports *aContext)

Nit: "*"s next to type.

::: layout/style/nsFontFaceLoader.cpp:309
(Diff revision 1)
> +{
> +  nsCOMPtr<nsIThreadRetargetableRequest> req = do_QueryInterface(aRequest);
> +  if (req) {
> +    nsCOMPtr<nsIEventTarget> sts =
> +      do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
> +    mozilla::Unused << NS_WARN_IF(NS_FAILED(req->RetargetDeliveryTo(sts)));

Can drop the "mozilla::" in here.

So just to confirm my understanding: this call causes nsIStreamListener::OnDataAvailable to be called on the stream transport service thread, but the stream loader will call back to nsFontFaceLoader::OnStreamComplete back on the main thread?

::: layout/style/nsFontFaceLoader.cpp:315
(Diff revision 1)
> +nsFontFaceLoader::OnStopRequest(nsIRequest *aRequest,
> +                                nsISupports *aContext,

Nit: and here.

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8913501 [details]
Bug 1381434 - off-main-thread loading web font

https://reviewboard.mozilla.org/r/184852/#review190366

::: layout/style/nsFontFaceLoader.cpp:309
(Diff revision 1)
> +{
> +  nsCOMPtr<nsIThreadRetargetableRequest> req = do_QueryInterface(aRequest);
> +  if (req) {
> +    nsCOMPtr<nsIEventTarget> sts =
> +      do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
> +    mozilla::Unused << NS_WARN_IF(NS_FAILED(req->RetargetDeliveryTo(sts)));

Yes `OnStreamComplete` is still called on main thread. Only OnDataAvailable is moved to stream transport thread if retargeted successfully.
I'll put an `MOZ_ASSERT(NS_IsMainThread())` at the beginning of `OnStreamComplete`, `OnStartRequest`, and `OnStopRequest` so that people can understand the running thread easily.

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8913501 [details]
Bug 1381434 - off-main-thread loading web font

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184852/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f6d9016ac75a
off-main-thread loading web font r=heycam

Comment 8

[Jet Villegas (inactive)]

\o/ Thanks, SC & heycam!

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 10

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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