TabGroup::IsBackground() takes 300ms in the content process with telemetry enabled when running

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: Ehsan, Assigned: mystor)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed, firefox56 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(2 attachments, 1 obsolete attachment)

It is just too expensive.  It needs to be cheaper please.
Created attachment 8875828 [details] [diff] [review]
Manage TabGroup::IsBackground passively to avoid Runnable overhead

MozReview-Commit-ID: 9HVvV1omF8G
Attachment #8875828 - Flags: review?(ehsan)
Comment on attachment 8875828 [details] [diff] [review]
Manage TabGroup::IsBackground passively to avoid Runnable overhead

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

::: dom/base/TabGroup.cpp
@@ +170,5 @@
>    MOZ_ASSERT(mWindows.Contains(aWindow));
>    mWindows.RemoveElement(aWindow);
>  
> +  if (!aWindow->IsBackground()) {
> +    mForegroundCount--;

Please MOZ_DIAGNOSTIC_ASSERT(mForegroundCount > 0); before doing so.

@@ +272,5 @@
> +{
> +  MOZ_RELEASE_ASSERT(NS_IsMainThread());
> +
> +  if (aIsNowBackground) {
> +    MOZ_ASSERT(mForegroundCount > 0);

I think this needs to be a diagnostic assert cause if we get this wrong we'll underflow...

@@ +285,5 @@
>  {
>    MOZ_RELEASE_ASSERT(NS_IsMainThread());
>  
> +#ifdef DEBUG
> +  uint32_t calculated = 0;

Nit: how about calling this foregrounded? :-)

::: dom/base/nsGlobalWindow.cpp
@@ +10616,2 @@
>    bool resetTimers = (!aIsBackground && AsOuter()->IsBackground());
> +  mIsBackground = aIsBackground;

Do you mind please putting the if branch and the mIsBackground assignment in a helper?  It seems like this and the nsGlobalWindow::SetDocShell() function above could use the helper directly which would be a bit less error prone.

@@ +15179,5 @@
>      MOZ_ASSERT_IF(testGroup, testGroup == toJoin);
>  #endif
>  
>      mTabGroup = mozilla::dom::TabGroup::Join(AsOuter(), toJoin);
> +    if (!mIsBackground) {

per IRC conversation, this needs to move to Join().
Attachment #8875828 - Flags: review?(ehsan) → review-
Created attachment 8875848 [details] [diff] [review]
Manage TabGroup::IsBackground passively to avoid Runnable overhead

MozReview-Commit-ID: 9HVvV1omF8G
Attachment #8875848 - Flags: review?(ehsan)
Attachment #8875828 - Attachment is obsolete: true
Comment on attachment 8875848 [details] [diff] [review]
Manage TabGroup::IsBackground passively to avoid Runnable overhead

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

Thanks!
Attachment #8875848 - Flags: review?(ehsan) → review+

Comment 5

2 months ago
Pushed by michael@thelayzells.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5a10e1d826
Manage TabGroup::IsBackground passively to avoid Runnable overhead, r=ehsan
I think this needs to be backported to beta.
Created attachment 8877249 [details] [diff] [review]
BETA Manage TabGroup::IsBackground passively to avoid Runnable overhead

MozReview-Commit-ID: 9HVvV1omF8G
Comment on attachment 8877249 [details] [diff] [review]
BETA Manage TabGroup::IsBackground passively to avoid Runnable overhead

Approval Request Comment
[Feature/Bug causing the regression]: bug 1322184
[User impact if declined]: Slow computations of TabGroup::IsBackground used for Telemetry
[Is this code covered by automated tests?]: It is used by every event dispatch
[Has the fix been verified in Nightly?]: Just landed
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: medium-low risk
[Why is the change risky/not risky?]: Changes hot code path to use cached value. Shouldn't have any security impact if caching gotten wrong.
[String changes made/needed]: None
Attachment #8877249 - Flags: approval-mozilla-beta?

Comment 9

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cc5a10e1d826
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8877249 [details] [diff] [review]
BETA Manage TabGroup::IsBackground passively to avoid Runnable overhead

fix perf regression in 55, let's get this in 55b2
Attachment #8877249 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 11

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/54d9fb0f5aac
status-firefox55: --- → fixed
(In reply to Michael Layzell [:mystor] from comment #8)
> [Is this code covered by automated tests?]: It is used by every event
> dispatch
> [Has the fix been verified in Nightly?]: Just landed
> [Needs manual test from QE? If yes, steps to reproduce]: No 

Setting qe-verify- based on Michael's assessment on manual testing needs.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.