Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 12 additions & 16 deletions src/actions/profile-view.js
Original file line number Diff line number Diff line change
Expand Up @@ -380,14 +380,13 @@ export function selectTrack(
);
}

const doesNextTrackHaveSelectedTab = getThreadSelectors(selectedThreadIndex)
.getUsefulTabs(getState())
.includes(selectedTab);

if (!doesNextTrackHaveSelectedTab) {
const visibleTabs = getThreadSelectors(selectedThreadIndex).getUsefulTabs(
getState()
);
if (!visibleTabs.includes(selectedTab)) {
// If the user switches to another track that doesn't have the current
// selectedTab then switch to the calltree.
selectedTab = 'calltree';
// selectedTab then switch to the first tab.
selectedTab = visibleTabs[0];
}

let selectedThreadIndexes = new Set(getSelectedThreadIndexes(getState()));
Expand Down Expand Up @@ -546,16 +545,13 @@ export function selectActiveTabTrack(
);
}

const doesNextTrackHaveSelectedTab = getThreadSelectors(
selectedThreadIndexes
)
.getUsefulTabs(getState())
.includes(selectedTab);

if (!doesNextTrackHaveSelectedTab) {
const visibleTabs = getThreadSelectors(selectedThreadIndexes).getUsefulTabs(
getState()
);
if (!visibleTabs.includes(selectedTab)) {
// If the user switches to another track that doesn't have the current
// selectedTab then switch to the calltree.
selectedTab = 'calltree';
// selectedTab then switch to the first tab.
selectedTab = visibleTabs[0];
}

if (
Expand Down
6 changes: 6 additions & 0 deletions src/app-logic/tabs-handling.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,9 @@ export const tabsWithTitleL10nIdArray: $ReadOnlyArray<TabsWithTitleL10nId> =
name: tabSlug,
title: tabsWithTitleL10nId[tabSlug],
}));

export const tabsShowingSampleData: $ReadOnlyArray<TabSlug> = [
'calltree',
'flame-graph',
'stack-chart',
];
41 changes: 33 additions & 8 deletions src/components/shared/SampleTooltipContents.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ import * as React from 'react';

import { Backtrace } from './Backtrace';
import { TooltipDetailSeparator } from '../tooltip/TooltipDetails';
import { getCategoryPairLabel } from 'firefox-profiler/profile-logic/profile-data';
import {
getCategoryPairLabel,
getFuncNamesAndOriginsForPath,
convertStackToCallNodeAndCategoryPath,
} from 'firefox-profiler/profile-logic/profile-data';
import {
formatMilliseconds,
formatPercent,
Expand All @@ -20,6 +24,7 @@ import type {
Thread,
} from 'firefox-profiler/types';
import type { CpuRatioInTimeRange } from './thread/ActivityGraphFills';
import { ensureExists } from '../../utils/flow';

type CPUProps = CpuRatioInTimeRange;

Expand Down Expand Up @@ -52,7 +57,6 @@ class SampleTooltipCPUContents extends React.PureComponent<CPUProps> {
<div className="tooltipDetails">
<div className="tooltipLabel">CPU:</div>
<div>{cpuUsageAndPercentage}</div>
<TooltipDetailSeparator />
</div>
);
}
Expand Down Expand Up @@ -117,6 +121,22 @@ export class SampleTooltipContents extends React.PureComponent<Props> {
categories,
implementationFilter,
} = this.props;

const { samples, stackTable } = rangeFilteredThread;
const stackIndex = samples.stack[sampleIndex];
const hasSamples = samples.length > 0 && stackTable.length > 1;
let hasStack = false;
if (hasSamples) {
const stack = getFuncNamesAndOriginsForPath(
convertStackToCallNodeAndCategoryPath(
rangeFilteredThread,
ensureExists(stackIndex)
),
rangeFilteredThread
);
hasStack = stack.length > 1 || stack[0].funcName !== '(root)';
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good idea to hide the stacks if we only have (root)! It wasn't looking good anyways.


return (
<>
{cpuRatioInTimeRange === null ? null : (
Expand All @@ -125,12 +145,17 @@ export class SampleTooltipContents extends React.PureComponent<Props> {
timeRange={cpuRatioInTimeRange.timeRange}
/>
)}
<SampleTooltipRestContents
sampleIndex={sampleIndex}
rangeFilteredThread={rangeFilteredThread}
categories={categories}
implementationFilter={implementationFilter}
/>
{hasStack && cpuRatioInTimeRange !== null ? (
<TooltipDetailSeparator />
) : null}
{!hasStack ? null : (
<SampleTooltipRestContents
sampleIndex={sampleIndex}
rangeFilteredThread={rangeFilteredThread}
categories={categories}
implementationFilter={implementationFilter}
/>
)}
</>
);
}
Expand Down
39 changes: 37 additions & 2 deletions src/selectors/per-thread/composed.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@
// @flow
import { createSelector } from 'reselect';

import { tabSlugs, type TabSlug } from '../../app-logic/tabs-handling';
import {
tabSlugs,
type TabSlug,
tabsShowingSampleData,
} from '../../app-logic/tabs-handling';

import type {
Selector,
Expand All @@ -22,6 +26,8 @@ import type {
StackTimingByDepth,
} from '../../profile-logic/stack-timing';

import { ensureExists } from '../../utils/flow';

/**
* Infer the return type from the getStackAndSampleSelectorsPerThread function. This
* is done that so that the local type definition with `Selector<T>` is the canonical
Expand Down Expand Up @@ -59,7 +65,15 @@ export function getComposedSelectorsPerThread(
threadSelectors.getThread,
threadSelectors.getIsNetworkChartEmptyInFullRange,
threadSelectors.getJsTracerTable,
({ processType }, isNetworkChartEmpty, jsTracerTable) => {
(thread, isNetworkChartEmpty, jsTracerTable) => {
const {
processType,
samples,
stackTable,
stringTable,
frameTable,
funcTable,
} = thread;
if (processType === 'comparison') {
// For a diffing tracks, we display only the calltree tab for now, because
// other views make no or not much sense.
Expand All @@ -76,6 +90,27 @@ export function getComposedSelectorsPerThread(
if (!jsTracerTable) {
visibleTabs = visibleTabs.filter((tabSlug) => tabSlug !== 'js-tracer');
}
let hasSamples = samples.length > 0 && stackTable.length > 0;
if (hasSamples) {
const stackIndex = ensureExists(samples.stack[0]);
if (stackTable.prefix[stackIndex] === null) {
// There's only a single stack frame, check if it's '(root)'.
const frameIndex = stackTable.frame[stackIndex];
const funcIndex = frameTable.func[frameIndex];
const stringIndex = funcTable.name[funcIndex];
if (stringTable.getString(stringIndex) === '(root)') {
// If the first sample's stack is only the root, check if any other
// sample is different.
hasSamples = samples.stack.some((s) => s !== stackIndex);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this looks a bit costly, but probably it's going to return early after two iterations for most of our profiles. And not sure how else we can do it without looping. I think it's okay to keep like this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be faster to:

  1. get the string index for (root) outside of the loop
  2. compare string indexes here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .some call (assuming that's what is being called 'loop' in these comments) should only be done for profiles where the first sample contains a single frame, and then we only compare one integer with an array of integers. That should be fast enough, and we will only compare the entire array of integers (one per sample) when the profile was captured with nostacksampling and all the sample contain only "(root)".

}
}
}

if (!hasSamples) {
visibleTabs = visibleTabs.filter(
(tabSlug) => !tabsShowingSampleData.includes(tabSlug)
);
}
return visibleTabs;
}
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -298,10 +298,10 @@ exports[`SampleTooltipContents renders the sample with "variable CPU cycles" CPU
<div>
50% (average over 1.0ms)
</div>
<div
class="tooltipDetailSeparator"
/>
</div>
<div
class="tooltipDetailSeparator"
/>
<div
class="tooltipDetails"
>
Expand Down Expand Up @@ -410,10 +410,10 @@ exports[`SampleTooltipContents renders the sample with ns CPU usage information
<div>
70% (average over 1.0ms)
</div>
<div
class="tooltipDetailSeparator"
/>
</div>
<div
class="tooltipDetailSeparator"
/>
<div
class="tooltipDetails"
>
Expand Down Expand Up @@ -522,10 +522,10 @@ exports[`SampleTooltipContents renders the sample with µs CPU usage information
<div>
45% (average over 1.0ms)
</div>
<div
class="tooltipDetailSeparator"
/>
</div>
<div
class="tooltipDetailSeparator"
/>
<div
class="tooltipDetails"
>
Expand Down
14 changes: 10 additions & 4 deletions src/test/store/profile-view.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -441,12 +441,12 @@ describe('actions/ProfileView', function () {
it('can switch back to the thread, which remembers the last viewed panel', function () {
const profile = getNetworkTrackProfile();
const { dispatch, getState } = storeWithProfile(profile);
dispatch(App.changeSelectedTab('flame-graph'));
dispatch(App.changeSelectedTab('marker-table'));
expect(UrlStateSelectors.getSelectedThreadIndexes(getState())).toEqual(
new Set([0])
);
expect(UrlStateSelectors.getSelectedTab(getState())).toEqual(
'flame-graph'
'marker-table'
);
dispatch(ProfileView.selectTrack(networkTrack, 'none'));
expect(UrlStateSelectors.getSelectedThreadIndexes(getState())).toEqual(
Expand All @@ -460,7 +460,7 @@ describe('actions/ProfileView', function () {
new Set([0])
);
expect(UrlStateSelectors.getSelectedTab(getState())).toEqual(
'flame-graph'
'marker-table'
);
});
});
Expand Down Expand Up @@ -519,9 +519,15 @@ describe('actions/ProfileView', function () {
expect(UrlStateSelectors.getSelectedTab(getState())).toEqual(
'calltree'
);
// The thread with the memory track doesn't have samples, so switch to
// a tab that exists even for tables without samples.
dispatch(App.changeSelectedTab('marker-table'));
expect(UrlStateSelectors.getSelectedTab(getState())).toEqual(
'marker-table'
);
dispatch(ProfileView.selectTrack(memoryTrackReference, 'none'));
expect(UrlStateSelectors.getSelectedTab(getState())).toEqual(
'calltree'
'marker-table'
);
});
});
Expand Down
28 changes: 19 additions & 9 deletions src/test/store/useful-tabs.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { selectedThreadSelectors } from '../../selectors/per-thread';

import { storeWithProfile } from '../fixtures/stores';
import {
getMarkerTableProfile,
getProfileFromTextSamples,
getProfileWithMarkers,
getNetworkMarkers,
Expand All @@ -33,9 +34,6 @@ describe('getUsefulTabs', function () {
const profile = getProfileWithMarkers(getNetworkMarkers());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a small tests for this new behavior too? We already see that in some of the tests, we don't have the sample related tabs anymore since the profiles only include markers, but it could be nice to add a simple test that checks this new behavior only.
The test name could be something like does not show sample related tabs when there is no sample data in the profile and it could test a profile like const profile = getMarkerTableProfile() or getProfileWithMarkers(getNetworkMarkers());

const { getState } = storeWithProfile(profile);
expect(selectedThreadSelectors.getUsefulTabs(getState())).toEqual([
'calltree',
'flame-graph',
'stack-chart',
'marker-chart',
'marker-table',
'network-chart',
Expand All @@ -46,9 +44,6 @@ describe('getUsefulTabs', function () {
const profile = getProfileWithJsTracerEvents([['A', 0, 10]]);
const { getState } = storeWithProfile(profile);
expect(selectedThreadSelectors.getUsefulTabs(getState())).toEqual([
'calltree',
'flame-graph',
'stack-chart',
'marker-chart',
'marker-table',
'js-tracer',
Expand Down Expand Up @@ -97,12 +92,27 @@ describe('getUsefulTabs', function () {

// Check the tabs and make sure that the network chart is there.
expect(selectedThreadSelectors.getUsefulTabs(getState())).toEqual([
'calltree',
'flame-graph',
'stack-chart',
'marker-chart',
'marker-table',
'network-chart',
]);
});

it('hides sample related tabs when there is no sample data in the profile', function () {
const profile = getMarkerTableProfile();
const { getState } = storeWithProfile(profile);
expect(selectedThreadSelectors.getUsefulTabs(getState())).toEqual([
'marker-chart',
'marker-table',
]);
});

it('hides sample related tabs when samples contain only the (root) frame', function () {
const { profile } = getProfileFromTextSamples('(root)');
const { getState } = storeWithProfile(profile);
expect(selectedThreadSelectors.getUsefulTabs(getState())).toEqual([
'marker-chart',
'marker-table',
]);
});
});