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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

It is just too expensive.  It needs to be cheaper please.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Manage TabGroup::IsBackground passively to avoid Runnable overhead

MozReview-Commit-ID: 9HVvV1omF8G

Comment 2

[(no longer active)]

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

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Manage TabGroup::IsBackground passively to avoid Runnable overhead

MozReview-Commit-ID: 9HVvV1omF8G

Comment 4

[(no longer active)]

Comment on attachment 8875848 [details] [diff] [review]
Manage TabGroup::IsBackground passively to avoid Runnable overhead

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

Thanks!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5a10e1d826
Manage TabGroup::IsBackground passively to avoid Runnable overhead, r=ehsan

Comment 6

[(no longer active)]

I think this needs to be backported to beta.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: BETA Manage TabGroup::IsBackground passively to avoid Runnable overhead

MozReview-Commit-ID: 9HVvV1omF8G

Comment 8

[Nika Layzell [:nika] (ni? for response)]

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[Julien Cristau [:jcristau]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/54d9fb0f5aac

Comment 12

[Andrei Vaida [:avaida]]

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