Closed Bug 1381434 Opened 2 years ago Closed 2 years ago

use HTTP OMT data delivery while loading fontface

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: schien, Assigned: schien)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

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
Priority: -- → P3
(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?
Flags: needinfo?(schien)
Looks like nsStreamLoader is simple enough to do OMT already. Let's see how this quick patch goes.
Assignee: nobody → schien
Flags: needinfo?(schien)
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.
Attachment #8913501 - Flags: review?(cam) → review+
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.
Pushed by schien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f6d9016ac75a
off-main-thread loading web font r=heycam
\o/ Thanks, SC & heycam!
https://hg.mozilla.org/mozilla-central/rev/f6d9016ac75a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Depends on: 1363945
Depends on: 1363940
Depends on: 1430755
You need to log in before you can comment on or make changes to this bug.