1310880 - Create a special BackgroundHangMonitor specifically for content process main thread force paints

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

In bug 1303077, I added an annotation, "ForcePaintInProgress", for any hangs that occur after a paint is forced.

The mechanism isn't perfect though. The BackgroundHangMonitor that we're annotating cares about when we stop and start processing events. For getting rid of the spinner in bug 1300411 (which all of this is about), what we care most about are hangs that occur between the point where we ask the ProcessHangMonitor to force paint, and when the paint actually occurs.

I talked to billm about this, and what we want to do is create a BackgroundHangMonitor that has its NotifyActivity called when a force paint is requested, and NotifyWait called when the paint is completed. The BackgroundHangThread will poll at interval during that window if it exceeds a certain threshold (I guess we'll pick some numbers... maybe 128ms).

What's particularly nice about this is that we will then have a special named "thread" inside threadHangStats that will just contain hangs that occurred during tab switch, and nothing else. No filtering necessary.

Reading the documentation at http://searchfox.org/mozilla-central/rev/d96317a351af8aa78ab9847e7feed964bbaac7d7/xpcom/threads/BackgroundHangMonitor.h#28, I think this should be possible.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Note that if we go for this, we can safely back out bug 1303077.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Hey jimchen, do you know if what billm and I are looking for is possible?

We're basically looking for a dedicated BackgroundHangMonitor for the main thread in the content process that starts monitoring for hangs once force paint is requested, and until the paint is completed. This is in tandem to the already-existing BackgroundHangMonitor that's running on the main thread in the content process, monitoring events.

Note that the notification to force paint comes in off of the main thread, but the notification that the paint has occurred occurs on the main thread.

Before I start really diving into this, is what we're looking for even possible / advisable with our current BHR infrastructure?

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

I think you can get away with creating a special BackgroundHangMonitor/BackgroundHangThread pair for forced paints.

Basically, create a special BHT on the main thread, and use it to initialize a new BHM. To avoid clashing with the normal BHT, you'll need to set the mThreadID to null and bypass the TLS (coincidentally there's a bug now that makes BHT never use the TLS, but we should fix that bug).

To BackgroundHangManager, it would look like a separate thread and it won't interfere with the normal BHT associated with the main thread.

That new BHM would let you call NotifyActivity/NotifyWait on it, but you'll need to add synchronization because you're calling from two threads.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread.

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

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints.

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

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8802279 [details]
Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread.

https://reviewboard.mozilla.org/r/86680/#review85660

Sorry for the drive-by, but I was reading this over out of curiousity. I think you might want to modify FindThread to skip over private threads.

::: xpcom/threads/BackgroundHangMonitor.cpp:600
(Diff revision 1)
> -                                             uint32_t aMaxTimeoutMs)
> -  : mThread(BackgroundHangThread::FindThread())
> +                                             uint32_t aMaxTimeoutMs,
> +                                             bool aUsePrivateThread)
>  {
>  #ifdef MOZ_ENABLE_BACKGROUND_HANG_MONITOR
> -  if (!BackgroundHangManager::sDisabled && !mThread) {
> -    mThread = new BackgroundHangThread(aName, aTimeoutMs, aMaxTimeoutMs);
> +  if (!BackgroundHangManager::sDisabled) {
> +    mThread = BackgroundHangThread::FindThread();

Probably want to avoid FindThread if aUsePrivateThread. It's just wasted time.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8802280 [details]
Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints.

https://reviewboard.mozilla.org/r/86682/#review85662

I'm not too happy with the locking here. The locking should happen inside BHR. I think it would be better if BackgroundHangThread::NotifyActivity/NotifyWait took manager->mLock. You'll have to shift around the code in NotifyWait to make sure it only takes the lock once. It looks like the original code tried to avoid taking a lock, but that seems silly to me. Taking an uncontended lock is very fast, and we're only doing this twice per event.

Long term, we should also look into changing this code so it doesn't use PR_Interrupt. It would be better to notify the mLock monitor.

::: dom/ipc/ProcessHangMonitor.cpp:119
(Diff revision 1)
>  
>   private:
>    void ShutdownOnThread();
>  
>    static Atomic<HangMonitorChild*> sInstance;
> +  BackgroundHangMonitor* mForcePaintMonitor;

Please use UniquePtr here.

::: dom/ipc/ProcessHangMonitor.cpp:286
(Diff revision 1)
>    mContext = danger::GetJSContext();
> +  mForcePaintMonitor =
> +    new mozilla::BackgroundHangMonitor("Gecko_Child_ForcePaint",
> +                                       128, /* ms timeout for microhangs */
> +                                       4096 /* ms timeout for permahangs */,
> +                                       true /* use private thread */);

Might be good to have an enum for this arg so we pass in something more informative like USE_PRIVATE_THREAD.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802279 [details]
Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread.

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

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802280 [details]
Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints.

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

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8802280 [details]
Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints.

https://reviewboard.mozilla.org/r/86682/#review86044

Looks great. Thanks.

::: dom/ipc/ProcessHangMonitor.cpp:285
(Diff revision 2)
>    MOZ_RELEASE_ASSERT(NS_IsMainThread());
>    mContext = danger::GetJSContext();
> +  mForcePaintMonitor =
> +    MakeUnique<mozilla::BackgroundHangMonitor>("Gecko_Child_ForcePaint",
> +                                               128, /* ms timeout for microhangs */
> +                                               4096 /* ms timeout for permahangs */,

Let's make the permahang time a little longer. Maybe 8 seconds? I think we cap the hang duration at that value, so I would like a little more resolution.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802279 [details]
Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread.

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

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802280 [details]
Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints.

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

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8802279 [details]
Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread.

https://reviewboard.mozilla.org/r/86680/#review86052

r+ with comments addressed.

::: xpcom/threads/BackgroundHangMonitor.h:119
(Diff revision 2)
>    static bool ShouldDisableOnBeta(const nsCString &);
>    static bool DisableOnBeta();
>  
>  public:
>    static const uint32_t kNoTimeout = 0;
> +  enum ThreadType {

Nit: add some comments

::: xpcom/threads/BackgroundHangMonitor.h:187
(Diff revision 2)
> +   * @param aUsePrivateThread (optional) True if a dedicated
> +   *  BackgroundHangThread should be created for this monitor. Otherwise,
> +   *  BackgroundHangMonitors will share the same BackgroundHangThread
> +   *  per monitored thread.

Nit: update the comment.

::: xpcom/threads/BackgroundHangMonitor.cpp:190
(Diff revision 2)
>    // Report a hang; aManager->mLock IS locked
>    Telemetry::HangHistogram& ReportHang(PRIntervalTime aHangTime);
>    // Report a permanent hang; aManager->mLock IS locked
>    void ReportPermaHang();
>    // Called by BackgroundHangMonitor::NotifyActivity
>    void NotifyActivity();

Inline `NotifyActivity()` within the class (similar to `NotifyWait()`) now that you moved its code to `Update()`.

::: xpcom/threads/BackgroundHangMonitor.cpp:198
(Diff revision 2)
>    {
> -    NotifyActivity();
> +    MonitorAutoLock autoLock(mManager->mLock);
> +    Update();
>      mWaiting = true;
>    }
> +  void Update();

Make `Update()` private please.

::: xpcom/threads/BackgroundHangMonitor.cpp:376
(Diff revision 2)
>    , mHanging(false)
>    , mWaiting(true)
> +  , mThreadType(aThreadType)
>    , mStats(aName)
>  {
> -  if (sTlsKeyInitialized) {
> +  if (sTlsKeyInitialized && IsShared()) {

Can you also fix the TLS bug? Right now TLS is never used because `sTlsKeyInitialized` is never set.

::: xpcom/threads/BackgroundHangMonitor.cpp:621
(Diff revision 2)
> +    });
> +
> +    if (aThreadType == THREAD_SHARED) {
> +      mThread = BackgroundHangThread::FindThread();
> +      if (mThread) {
> +        threadCreationGuard.release();

This is a bit of a brain twister. Why not just say `mThread(aThreadType == THREAD_SHARED ? BackgroundHangThread::FindThread() : nullptr)` in the initializer list? Note that `mThread` is left uninitialized if `BackgroundHangManager::sDisabled` is true.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Ah I was looking at an old rev. You did change the comments.

Also, billm is right that you should change FindThread() to not look at private threads.

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8802280 [details]
Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints.

https://reviewboard.mozilla.org/r/86682/#review86058

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802279 [details]
Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread.

https://reviewboard.mozilla.org/r/86680/#review86052

> This is a bit of a brain twister. Why not just say `mThread(aThreadType == THREAD_SHARED ? BackgroundHangThread::FindThread() : nullptr)` in the initializer list? Note that `mThread` is left uninitialized if `BackgroundHangManager::sDisabled` is true.

Oh wow, yeah, that's way simpler. Good suggestion, thanks!

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802279 [details]
Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86680/diff/3-4/

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802280 [details]
Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/86682/diff/3-4/

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Thanks for the super fast reviews, jchen and billm.

jchen - I've got one final iteration up. I know I've got carry-forward powers, so it's still r+'d and landable, but I think I'd feel more confident that I interpreted your suggestions correctly if you gave it one more look.

Latest iteration look okay?

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8802279 [details]
Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread.

https://reviewboard.mozilla.org/r/86680/#review86082

::: xpcom/threads/BackgroundHangMonitor.h:121
(Diff revision 4)
>  
>  public:
>    static const uint32_t kNoTimeout = 0;
> +  enum ThreadType {
> +    // For a new BackgroundHangMonitor for thread T, only create a new
> +    // monitoring thread for T if onen doesn't already exist. If one does,

one is misspelled as "onen"

::: xpcom/threads/BackgroundHangMonitor.cpp:524
(Diff revision 4)
>    PRThread* threadID = PR_GetCurrentThread();
>    // Lock thread list for traversal
>    MonitorAutoLock autoLock(manager->mLock);
>    for (BackgroundHangThread* thread = manager->mHangThreads.getFirst();
>         thread; thread = thread->getNext()) {
> -    if (thread->mThreadID == threadID) {
> +    if (thread->mThreadID == threadID && !thread->IsShared()) {

I think you meant `&& thread->IsShared()` (i.e. return a thread if it's _not_ private)

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Looks great otherwise. Thanks!

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8802279 [details]
Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread.

https://reviewboard.mozilla.org/r/86680/#review86082

> I think you meant `&& thread->IsShared()` (i.e. return a thread if it's _not_ private)

Good eye!

Comment 23

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a1d7f6e18f9434c4be195a986571cc83e21c9dd
Bug 1310880 - Allow a BackgroundHangMonitor to have its own private BackgroundHangThread. r=jchen

https://hg.mozilla.org/integration/mozilla-inbound/rev/48071b5d0cb211139be1ce7af508861cc79ac442
Bug 1310880 - Add a specialized BackgroundHangMonitor for content process force paints. r=jchen,billm

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1a1d7f6e18f9
https://hg.mozilla.org/mozilla-central/rev/48071b5d0cb2

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I realized I missed a problem here. If we get a force paint message, there's no guarantee that JS is actually running. The next paint we do could be through our normal event loop. We should move the NotifyWait() call in RecvSetDocShellIsActive I think, although we'll need to be careful to run it only if there was a previous NotifyActive(). Sorry about this Mike.