[Shutdown] Font loading on shutdown

RESOLVED FIXED in mozilla28

Status

()

Core
Graphics: Text
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: BenWa, Assigned: jfkthame)

Tracking

(Blocks: 1 bug)

Trunk
mozilla28
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
Profile: http://people.mozilla.com/~bgirard/cleopatra/#report=f7fc1b232203cb02a0c874ee10673d7936dc4736&search=Timer

Looks like we don't abort gfxFontInfoLoader::LoaderTimerCallback on shutdown. In this case we end up processing fonts during nsThread::Shutdown which is a waste.

Any idea on when this timer should be aborted in the shutdown sequence?
(Reporter)

Comment 1

5 years ago
Created attachment 704704 [details] [diff] [review]
patch
(Reporter)

Comment 2

5 years ago
Created attachment 707268 [details] [diff] [review]
Stop background font loading on quit-application-granted
Assignee: nobody → bgirard
Attachment #704704 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #707268 - Flags: review?(jdaggett)
(Assignee)

Comment 3

5 years ago
quit-application-granted may be to early for this, as shutdown may still be cancelled, afaict. Perhaps the quit-application notification would be better?

I wonder, though - do we really need the loose coupling provided by the notification/observer pattern, or could we reduce this to a simple function call on gfxPlatform?
(Reporter)

Comment 4

5 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #3)
> quit-application-granted may be to early for this, as shutdown may still be
> cancelled, afaict. Perhaps the quit-application notification would be better?

According to this page you are correct:
https://wiki.mozilla.org/App_quit_sequence

> 
> I wonder, though - do we really need the loose coupling provided by the
> notification/observer pattern, or could we reduce this to a simple function
> call on gfxPlatform?

Can you elaborate on simple function? We could have gfxPlatform::Shutdown call this but that happens during xpcom shutdown which is a bit later than quit-application giving this a chance to run if we spin the event loop. We could restore the timer if the application quit is canceled.
(Assignee)

Comment 5

5 years ago
Yes, gfxPlatform::Shutdown may be later than you want, but we could create a gfxPlatform::WillShutdown method (or something like that), and call that from the same place that quit-application is currently notified.

I realize that would just be duplicating what the notification/observer mechanism can achieve, but it seems like it'd actually be -less- code and complexity than creating the observer object, adding it to the observer list (and removing it later), etc.

Updated

5 years ago
Attachment #707268 - Flags: review?(jdaggett) → review?(jfkthame)
(Assignee)

Comment 6

5 years ago
I looked at this again, and I'm thinking that although it'd be a bit simpler on the gfx side to create a WillShutdown method and call that, we should probably just stay with the existing notification/observer pattern. We'd need to call the hypothetical gfxPlatform::WillShutdown from nsAppStartup::Quit() in toolkit/components/startup/nsAppStartup.cpp, but AFAICT the toolkit C++ code does not currently include any gfx headers, or link directly to anything in gfx. Violating that module separation just for this doesn't really seem justified.

So let's use something like your original patch, if you'd like to update it to use the quit-application notification.
(Reporter)

Comment 7

5 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #6)
> I looked at this again, and I'm thinking that although it'd be a bit simpler
> on the gfx side to create a WillShutdown method and call that, we should
> probably just stay with the existing notification/observer pattern. We'd
> need to call the hypothetical gfxPlatform::WillShutdown from
> nsAppStartup::Quit() in toolkit/components/startup/nsAppStartup.cpp, but
> AFAICT the toolkit C++ code does not currently include any gfx headers, or
> link directly to anything in gfx. Violating that module separation just for
> this doesn't really seem justified.

I don't really think this applies since we switch to libxul. But in any case maybe a hybrid approach is better. We could have gfxPlatform register for the right shutdown notification which will invoke WillShutdown that can clean up our active timer starting with this font timer.

> 
> So let's use something like your original patch, if you'd like to update it
> to use the quit-application notification.

Or I can keep this approach.
(Assignee)

Comment 8

5 years ago
(Sorry for the slow response here.)

Let's stay with an observer for an existing notification, I think. In which case I don't see much benefit in passing it via gfxPlatform for now, though that might change if we find a number of separate things in gfx that we'd like to clean up or halt at the same time.

Actually, would it be feasible to make the gfxFontLoader itself -be- the observer, rather than creating and managing an additional object? I admit I haven't thought too hard about that; maybe it would introduce other complications, but offhand it seems like it might be simpler.
(Assignee)

Updated

5 years ago
Duplicate of this bug: 859996
Per BenWa's IRC request, I let a profile run for ~5 minutes and I still see the stat64() on shutdown. I also captured a bunch of stat64() and getattrlist() on startup as well. There's lots of duplication there - I have 10 consecutive stat64() to Skia.ttf all happening within microseconds of one another!
(Assignee)

Comment 11

5 years ago
Hmm. In that case maybe we should un-dupe bug 859996 and try to investigate a bit further there. Could you get a backtrace for one of these shutdown stat()s (after waiting long enough for the post-startup loader to finish)?
I can obtain a stack if you really need me to. But, that's not something I do very often and I'd be very inefficient about it. I have a feeling a seasoned Gecko developer on OS X could reproduce this and obtain all the needed info in far less time than it would take me. If you really need me to get this for you, needinfo? me and I'll get around to it when I have time. Please also tell me how to ensure I'm waiting until the "post-startup loader" has finished because I have no clue what the "post-startup loader" is.
(Reporter)

Comment 13

4 years ago
Comment on attachment 707268 [details] [diff] [review]
Stop background font loading on quit-application-granted

Let's put the review on hold for now but this is still important to fix.
Attachment #707268 - Flags: review?(jfkthame)
(Assignee)

Comment 14

4 years ago
(In reply to Gregory Szorc [:gps] from comment #10)
> Per BenWa's IRC request, I let a profile run for ~5 minutes and I still see
> the stat64() on shutdown. I also captured a bunch of stat64() and
> getattrlist() on startup as well. There's lots of duplication there - I have
> 10 consecutive stat64() to Skia.ttf all happening within microseconds of one
> another!

I can't seem to reproduce this with a current Nightly build. E.g., running "sudo fs_usage | grep -i skia" and launching Nightly, I consistently see just 4 accesses to Skia:

15:01:09  getattrlist       /Library/Fonts/Skia.ttf      0.000010   firefox     
15:01:09  getattrlist       /Library/Fonts/Skia.ttf      0.000009   firefox     
15:01:09  stat64            /Library/Fonts/Skia.ttf      0.000003   firefox     
15:01:09  open              /Library/Fonts/Skia.ttf      0.000014   firefox     

This occurs shortly after startup, when the background font-info-loader runs. But I don't see anything resembling "10 consecutive stat64" calls. Are you still seeing this?
(Reporter)

Comment 15

4 years ago
Created attachment 820338 [details]
profile screenshot

Still there and quite noticable
(Assignee)

Comment 16

4 years ago
Yes, the shutdown case will be unchanged. But I was wondering about gps's report from comment 10 about hitting the same font file many times in succession. I'm not seeing that behavior, AFAICT.
(Assignee)

Comment 17

4 years ago
Created attachment 825189 [details] [diff] [review]
stop background font info loading when quit-application notification is observed

Here's an updated version of the patch that listens for the quit-application notification and cancels the loader; seems to work OK in my local testing, at least. While touching the gfxFontInfoLoader code, I also moved the body of some of its methods from gfxFontUtils.h to the .cpp file, as there's no reason for them to live in the header, AFAICS.
Attachment #825189 - Flags: review?(bgirard)
(Assignee)

Comment 18

4 years ago
Created attachment 825213 [details] [diff] [review]
stop background font info loading when quit-application notification is observed

Comparing to your original patch, I noticed that I'd omitted removing the observer in the gfxFontInfoLoader destructor. Currently, that's not actually relevant because our singleton gfxPlatformFontList object (the gfxFontInfoLoader subclass we use) isn't destroyed until very late during shutdown - after the observer service is long gone, I believe. However, on principle it seems better to include the proper cleanup here, in case usage patterns change in the future. Sorry for the churn.
Attachment #825213 - Flags: review?(bgirard)
(Assignee)

Updated

4 years ago
Attachment #825189 - Attachment is obsolete: true
Attachment #825189 - Flags: review?(bgirard)
(Reporter)

Comment 19

4 years ago
Comment on attachment 825213 [details] [diff] [review]
stop background font info loading when quit-application notification is observed

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

::: gfx/thebes/gfxFontUtils.cpp
@@ +1496,5 @@
> +    }
> +    mState = stateTimerOff;
> +    if (mTimer) {
> +        mTimer->Cancel();
> +    }

Perhaps here we should:
mTimer = nullptr;
or is the timer flipped on/off often?
Attachment #825213 - Flags: review?(bgirard) → review+
(Assignee)

Comment 20

4 years ago
(In reply to Benoit Girard (:BenWa) from comment #19)
> Perhaps here we should:
> mTimer = nullptr;

Yeah, we should; once we cancel the timer, we're not going to need it again. Added that, and pushed to inbound.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c51e46899a05

This should prevent us continuing to read more fonts during shutdown if the browser is closed shortly after startup.

Gregory, if you still see additional issues with excessive font-file access, as per comment 10, I think it'd be best to file a separate bug about that.
Assignee: bgirard → jfkthame
Target Milestone: --- → mozilla28
(Reporter)

Comment 21

4 years ago
Thanks Jonathan for finishing this!
https://hg.mozilla.org/mozilla-central/rev/c51e46899a05
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.