Closed
Bug 1367458
Opened 7 years ago
Closed 7 years ago
TabGroup::IsBackground() takes 300ms in the content process with telemetry enabled when running
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
People
(Reporter: ehsan.akhgari, Assigned: nika)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
7.69 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
7.69 KB,
patch
|
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
It is just too expensive. It needs to be cheaper please.
Assignee | ||
Comment 1•7 years ago
|
||
MozReview-Commit-ID: 9HVvV1omF8G
Attachment #8875828 -
Flags: review?(ehsan)
Reporter | ||
Comment 2•7 years ago
|
||
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-
Assignee | ||
Comment 3•7 years ago
|
||
MozReview-Commit-ID: 9HVvV1omF8G
Attachment #8875848 -
Flags: review?(ehsan)
Assignee | ||
Updated•7 years ago
|
Attachment #8875828 -
Attachment is obsolete: true
Reporter | ||
Comment 4•7 years ago
|
||
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+
Pushed by michael@thelayzells.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/cc5a10e1d826 Manage TabGroup::IsBackground passively to avoid Runnable overhead, r=ehsan
Reporter | ||
Comment 6•7 years ago
|
||
I think this needs to be backported to beta.
Assignee | ||
Comment 7•7 years ago
|
||
MozReview-Commit-ID: 9HVvV1omF8G
Assignee | ||
Comment 8•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cc5a10e1d826
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 10•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/54d9fb0f5aac
status-firefox55:
--- → fixed
Comment 12•7 years ago
|
||
(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-
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•