Skip to content

Commit

Permalink
Do not make the parent process visible by default when something is s…
Browse files Browse the repository at this point in the history
…elected in the tab selector (#5198)
  • Loading branch information
canova authored Nov 12, 2024
1 parent 73111ee commit 28e880e
Show file tree
Hide file tree
Showing 7 changed files with 59 additions and 14 deletions.
13 changes: 10 additions & 3 deletions src/actions/receive-profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -315,9 +315,10 @@ export function finalizeFullProfileView(
const hasUrlInfo = maybeSelectedThreadIndexes !== null;

const tabToThreadIndexesMap = getTabToThreadIndexesMap(getState());
const tabFilter = hasUrlInfo ? getTabFilter(getState()) : null;
const globalTracks = computeGlobalTracks(
profile,
hasUrlInfo ? getTabFilter(getState()) : null,
tabFilter,
tabToThreadIndexesMap
);
const localTracksByPid = computeLocalTracksByPid(
Expand Down Expand Up @@ -370,10 +371,13 @@ export function finalizeFullProfileView(
// This is the case for the initial profile load.
// We also get here if the URL info was ignored, for example if
// respecting it would have caused all threads to become hidden.
const includeParentProcessThreads = tabFilter === null;
hiddenTracks = computeDefaultHiddenTracks(
tracksWithOrder,
profile,
getThreadActivityScores(getState())
getThreadActivityScores(getState()),
// Only include the parent process if there is no tab filter applied.
includeParentProcessThreads
);
}

Expand Down Expand Up @@ -1877,10 +1881,13 @@ export function changeTabFilter(tabID: TabID | null): ThunkAction<void> {
// This is the case for the initial profile load.
// We also get here if the URL info was ignored, for example if
// respecting it would have caused all threads to become hidden.
const includeParentProcessThreads = tabID === null;
hiddenTracks = computeDefaultHiddenTracks(
tracksWithOrder,
profile,
getThreadActivityScores(getState())
getThreadActivityScores(getState()),
// Only include the parent process if there is no tab filter applied.
includeParentProcessThreads
);
}

Expand Down
46 changes: 41 additions & 5 deletions src/profile-logic/tracks.js
Original file line number Diff line number Diff line change
Expand Up @@ -818,14 +818,16 @@ export function tryInitializeHiddenTracksFromUrl(
export function computeDefaultHiddenTracks(
tracksWithOrder: TracksWithOrder,
profile: Profile,
threadActivityScores: Array<ThreadActivityScore>
threadActivityScores: Array<ThreadActivityScore>,
includeParentProcessThreads: boolean
): HiddenTracks {
return _computeHiddenTracksForVisibleThreads(
profile,
computeDefaultVisibleThreads(
profile,
tracksWithOrder,
threadActivityScores
threadActivityScores,
includeParentProcessThreads
),
tracksWithOrder
);
Expand Down Expand Up @@ -1054,7 +1056,8 @@ const IDLE_THRESHOLD_FRACTION = 0.05;
export function computeDefaultVisibleThreads(
profile: Profile,
tracksWithOrder: TracksWithOrder,
threadActivityScores: Array<ThreadActivityScore>
threadActivityScores: Array<ThreadActivityScore>,
includeParentProcessThreads: boolean
): Set<ThreadIndex> {
const threads = profile.threads;
if (threads.length === 0) {
Expand Down Expand Up @@ -1099,11 +1102,33 @@ export function computeDefaultVisibleThreads(
// to the thread with the most "sampleScore" activity.
// We keep all threads whose sampleScore is at least 5% of the highest
// sampleScore, and also any threads which are otherwise essential.
// We also remove the parent process if that was requested. That's why we
// have to ignore their scores while computing the highest score.
const highestSampleScore = Math.max(
...scores.map(({ score }) => score.sampleScore)
...scores.map(({ score }) => {
if (score.isInParentProcess && !includeParentProcessThreads) {
// Do not account for the parent process threads if we do not want to
// include them for the visible threads by default.
return 0;
}
return score.sampleScore;
})
);
const thresholdSampleScore = highestSampleScore * IDLE_THRESHOLD_FRACTION;
const finalList = top15.filter(({ score }) => {
const tryToHideList = [];
let finalList = top15.filter((activityScore) => {
const { score } = activityScore;
if (score.isInParentProcess && !includeParentProcessThreads) {
// We try to hide this thread that belongs to the parent process.
// But when we hide all the threads we might encounter that all the
// threads are hidden now. For cases like this we would like to keep a
// tryToHideList, so we can add them back if the track is completely empty.
if (score.sampleScore >= thresholdSampleScore) {
tryToHideList.push(activityScore);
}
return false;
}

if (score.isEssentialFirefoxThread) {
return true; // keep.
}
Expand All @@ -1113,13 +1138,22 @@ export function computeDefaultVisibleThreads(
return score.sampleScore >= thresholdSampleScore;
});

if (finalList.length === 0) {
// We tried to hide the main process threads, but this resulted us to have
// an empty list. Put them back.
finalList = tryToHideList;
}

return new Set(finalList.map(({ threadIndex }) => threadIndex));
}

export type ThreadActivityScore = {|
// Whether this thread is one of the essential threads that
// should always be kept (unless there's too many of them).
isEssentialFirefoxThread: boolean,
// Whether this thread belongs to the parent process. We do not want to show
// them by default if the tab selector is used.
isInParentProcess: boolean,
// Whether this thread should be kept even if it looks very idle,
// as long as there's a single sample with non-zero activity.
isInterestingEvenWithMinimalActivity: boolean,
Expand All @@ -1146,6 +1180,7 @@ export function computeThreadActivityScore(
maxCpuDeltaPerInterval: number | null
): ThreadActivityScore {
const isEssentialFirefoxThread = _isEssentialFirefoxThread(thread);
const isInParentProcess = thread.processType === 'default';
const isInterestingEvenWithMinimalActivity =
_isFirefoxMediaThreadWhichIsUsuallyIdle(thread);
const sampleScore = _computeThreadSampleScore(
Expand All @@ -1158,6 +1193,7 @@ export function computeThreadActivityScore(
: sampleScore;
return {
isEssentialFirefoxThread,
isInParentProcess,
isInterestingEvenWithMinimalActivity,
sampleScore,
boostedSampleScore,
Expand Down
2 changes: 1 addition & 1 deletion src/test/components/TabSelectorMenu.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ describe('app/TabSelectorMenu', () => {
// Note that the first thread will be visible too, because it's the parent
// process which we always include.
expect(getHumanReadableTracks(getState())).toEqual([
'show [thread GeckoMain default]',
'hide [thread GeckoMain default]',
'show [thread GeckoMain tab] SELECTED',
' - show [thread DOM Worker]',
' - show [thread Style]',
Expand Down
4 changes: 2 additions & 2 deletions src/test/components/TrackContextMenu.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -515,8 +515,8 @@ describe('timeline/TrackContextMenu', function () {
expect(getHumanReadableTracks(getState())).toEqual([
'show [thread GeckoMain default] SELECTED',
'hide [thread GeckoMain tab]',
' - show [thread DOM Worker]',
' - show [thread Style]',
' - hide [thread DOM Worker]',
' - hide [thread Style]',
]);
});

Expand Down
4 changes: 2 additions & 2 deletions src/test/components/__snapshots__/GlobalTrack.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ Process: \\"tab\\" (222)"
class="react-contextmenu-wrapper timelineTrackLabel timelineTrackLocalLabel timelineTrackLocalGrippy"
title="DOM Worker
Thread: \\"DOM Worker\\" (33)
Process: \\"default\\" (222)"
Process: \\"tab\\" (222)"
>
<button
class="timelineTrackNameButton"
Expand Down Expand Up @@ -233,7 +233,7 @@ Process: \\"default\\" (222)"
class="react-contextmenu-wrapper timelineTrackLabel timelineTrackLocalLabel timelineTrackLocalGrippy"
title="Style
Thread: \\"Style\\" (44)
Process: \\"default\\" (222)"
Process: \\"tab\\" (222)"
>
<button
class="timelineTrackNameButton"
Expand Down
2 changes: 1 addition & 1 deletion src/test/components/__snapshots__/LocalTrack.test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ exports[`timeline/LocalTrack with a thread track matches the snapshot of a local
class="react-contextmenu-wrapper timelineTrackLabel timelineTrackLocalLabel timelineTrackLocalGrippy"
title="DOM Worker
Thread: \\"DOM Worker\\" (33)
Process: \\"default\\" (222)"
Process: \\"tab\\" (222)"
>
<button
class="timelineTrackNameButton"
Expand Down
2 changes: 2 additions & 0 deletions src/test/fixtures/profiles/tracks.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,11 @@ export function getProfileWithNiceTracks(): Profile {

thread3.name = 'DOM Worker';
thread3.pid = '222';
thread3.processType = 'tab';
thread3.tid = 33;

thread4.name = 'Style';
thread4.processType = 'tab';
thread4.pid = '222';
thread4.tid = 44;
return profile;
Expand Down

0 comments on commit 28e880e

Please sign in to comment.