833087 - [Shutdown] Font loading on shutdown

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Benoit Girard (:BenWa)]

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?

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Stop background font loading on quit-application-granted

Comment 3

[Jonathan Kew [:jfkthame]]

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?

Comment 4

[Benoit Girard (:BenWa)]

(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.

Comment 5

[Jonathan Kew [:jfkthame]]

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.

Comment 6

[Jonathan Kew [:jfkthame]]

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.

Comment 7

[Benoit Girard (:BenWa)]

(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.

Comment 8

[Jonathan Kew [:jfkthame]]

(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.

Comment 10

[Gregory Szorc [:gps]]

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!

Comment 11

[Jonathan Kew [:jfkthame]]

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)?

Comment 12

[Gregory Szorc [:gps]]

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.

Comment 13

[Benoit Girard (:BenWa)]

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.

Comment 14

[Jonathan Kew [:jfkthame]]

(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?

Comment 15

[Benoit Girard (:BenWa)]

Attachment: profile screenshot

Still there and quite noticable

Comment 16

[Jonathan Kew [:jfkthame]]

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.

Comment 17

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: 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.

Comment 19

[Benoit Girard (:BenWa)]

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?

Comment 20

[Jonathan Kew [:jfkthame]]

(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.

Comment 21

[Benoit Girard (:BenWa)]

Thanks Jonathan for finishing this!

Comment 22

[Carsten Book [:Tomcat]]

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