From df6f593403a7d2301849c807241c76f6191a0d66 Mon Sep 17 00:00:00 2001 From: Greg Tatum Date: Fri, 12 Jul 2019 15:36:37 -0500 Subject: [PATCH 1/2] Add a mechanism to change the summary strategy and store it in the URL --- src/actions/profile-view.js | 17 +++++++++++++++ src/app-logic/url-handling.js | 8 ++++++++ src/profile-logic/profile-data.js | 20 +++++++++++++++++- src/reducers/url-state.js | 18 ++++++++++++++++ src/selectors/url-state.js | 3 +++ src/test/store/profile-view.test.js | 13 ++++++++++++ src/test/url-handling.test.js | 32 +++++++++++++++++++++++++++++ src/types/actions.js | 9 ++++++++ src/types/state.js | 2 ++ 9 files changed, 121 insertions(+), 1 deletion(-) diff --git a/src/actions/profile-view.js b/src/actions/profile-view.js index c02603ea3a..cf3fad48c7 100644 --- a/src/actions/profile-view.js +++ b/src/actions/profile-view.js @@ -40,6 +40,7 @@ import { objectShallowEquals } from '../utils/index'; import type { PreviewSelection, ImplementationFilter, + CallTreeSummaryStrategy, TrackReference, TimelineType, DataSource, @@ -939,6 +940,22 @@ export function changeImplementationFilter( }; } +export function changeCallTreeSummaryStrategy( + strategy: CallTreeSummaryStrategy +): Action { + sendAnalytics({ + hitType: 'event', + eventCategory: 'profile', + eventAction: 'change call tree summary strategy', + eventLabel: strategy, + }); + + return { + type: 'CHANGE_CALL_TREE_SUMMARY_STRATEGY', + strategy, + }; +} + export function changeInvertCallstack( invertCallstack: boolean ): ThunkAction { diff --git a/src/app-logic/url-handling.js b/src/app-logic/url-handling.js index 41c3a40fed..49c07acac5 100644 --- a/src/app-logic/url-handling.js +++ b/src/app-logic/url-handling.js @@ -17,6 +17,7 @@ import { toValidTabSlug, ensureExists, } from '../utils/flow'; +import { toValidCallTreeSummaryStrategy } from '../profile-logic/profile-data'; import { oneLine } from 'common-tags'; import type { UrlState } from '../types/state'; import type { DataSource } from '../types/actions'; @@ -105,6 +106,7 @@ type CallTreeQuery = {| search: string, // "js::RunScript" invertCallstack: null | void, implementation: string, + ctSummary: string, |}; type MarkersQuery = {| @@ -122,6 +124,7 @@ type StackChartQuery = {| search: string, // "js::RunScript" invertCallstack: null | void, implementation: string, + ctSummary: string, |}; type JsTracerQuery = {| @@ -242,6 +245,10 @@ export function urlStateToUrlObject(urlState: UrlState): UrlObject { urlState.profileSpecific.transforms[selectedThread] ) || undefined; } + query.ctSummary = + urlState.profileSpecific.callTreeSummaryStrategy === 'timing' + ? undefined + : urlState.profileSpecific.callTreeSummaryStrategy; break; } case 'marker-table': @@ -359,6 +366,7 @@ export function stateFromLocation( profileName: query.profileName, profileSpecific: { implementation, + callTreeSummaryStrategy: toValidCallTreeSummaryStrategy(query.ctSummary), invertCallstack: query.invertCallstack !== undefined, showJsTracerSummary: query.summary !== undefined, committedRanges: query.range ? parseCommittedRanges(query.range) : [], diff --git a/src/profile-logic/profile-data.js b/src/profile-logic/profile-data.js index 3f1fcfb970..04a538deeb 100644 --- a/src/profile-logic/profile-data.js +++ b/src/profile-logic/profile-data.js @@ -35,7 +35,10 @@ import { assertExhaustiveCheck } from '../utils/flow'; import type { Milliseconds, StartEndRange } from '../types/units'; import { timeCode } from '../utils/time-code'; import { hashPath } from '../utils/path'; -import type { ImplementationFilter } from '../types/actions'; +import type { + ImplementationFilter, + CallTreeSummaryStrategy, +} from '../types/actions'; import bisection from 'bisection'; import type { UniqueStringArray } from '../utils/unique-string-array'; @@ -685,6 +688,21 @@ export function toValidImplementationFilter( } } +export function toValidCallTreeSummaryStrategy( + strategy: mixed +): CallTreeSummaryStrategy { + switch (strategy) { + case 'timing': + case 'js-allocations': + case 'native-allocations': + return strategy; + default: + // Default to timing if the strategy is not recognized. This value can come + // from a user-generated URL. + return 'timing'; + } +} + export function filterThreadByImplementation( thread: Thread, implementation: string, diff --git a/src/reducers/url-state.js b/src/reducers/url-state.js index f49d5dd567..44cea2fbcc 100644 --- a/src/reducers/url-state.js +++ b/src/reducers/url-state.js @@ -14,6 +14,7 @@ import type { TransformStacksPerThread } from '../types/transforms'; import type { DataSource, ImplementationFilter, + CallTreeSummaryStrategy, TimelineType, } from '../types/actions'; import type { UrlState, Reducer } from '../types/state'; @@ -228,6 +229,22 @@ const implementation: Reducer = ( } }; +/** + * Represents the current strategy used to summarize numeric information in the call + * call tree. + */ +const callTreeSummaryStrategy: Reducer = ( + state = 'timing', + action +) => { + switch (action.type) { + case 'CHANGE_CALL_TREE_SUMMARY_STRATEGY': + return action.strategy; + default: + return state; + } +}; + const invertCallstack: Reducer = (state = false, action) => { switch (action.type) { case 'CHANGE_INVERT_CALLSTACK': @@ -381,6 +398,7 @@ const profileSpecific = combineReducers({ hiddenLocalTracksByPid, localTrackOrderByPid, implementation, + callTreeSummaryStrategy, invertCallstack, showJsTracerSummary, committedRanges, diff --git a/src/selectors/url-state.js b/src/selectors/url-state.js index 10c15fcfb8..87b2ddaa51 100644 --- a/src/selectors/url-state.js +++ b/src/selectors/url-state.js @@ -16,6 +16,7 @@ import type { TimelineType, DataSource, ImplementationFilter, + CallTreeSummaryStrategy, } from '../types/actions'; import type { TabSlug } from '../app-logic/tabs-handling'; import type { UrlState } from '../types/state'; @@ -45,6 +46,8 @@ export const getAllCommittedRanges: Selector = state => getProfileSpecificState(state).committedRanges; export const getImplementationFilter: Selector = state => getProfileSpecificState(state).implementation; +export const getCallTreeSummaryStrategy: Selector = state => + getProfileSpecificState(state).callTreeSummaryStrategy; export const getInvertCallstack: Selector = state => getProfileSpecificState(state).invertCallstack; export const getShowJsTracerSummary: Selector = state => diff --git a/src/test/store/profile-view.test.js b/src/test/store/profile-view.test.js index 271204fd20..6b3cae4dce 100644 --- a/src/test/store/profile-view.test.js +++ b/src/test/store/profile-view.test.js @@ -1544,3 +1544,16 @@ describe('counter selectors', function() { }); }); }); + +describe('call tree summary strategy', function() { + it('can change the call tree strategy', function() { + const { dispatch, getState } = storeWithProfile(); + expect(UrlStateSelectors.getCallTreeSummaryStrategy(getState())).toEqual( + 'timing' + ); + dispatch(ProfileView.changeCallTreeSummaryStrategy('js-allocations')); + expect(UrlStateSelectors.getCallTreeSummaryStrategy(getState())).toEqual( + 'js-allocations' + ); + }); +}); diff --git a/src/test/url-handling.test.js b/src/test/url-handling.test.js index f933a661b6..37360166ba 100644 --- a/src/test/url-handling.test.js +++ b/src/test/url-handling.test.js @@ -843,3 +843,35 @@ describe('compare', function() { expect(resultingUrl).toMatch(`profiles[]=${encodeURIComponent(url2)}`); }); }); + +describe('call tree summary strategy', function() { + const { getCallTreeSummaryStrategy } = urlStateReducers; + + it('defaults to timing', function() { + const { getState } = _getStoreWithURL(); + expect(getCallTreeSummaryStrategy(getState())).toEqual('timing'); + }); + + it('can be js allocations', function() { + const { getState } = _getStoreWithURL({ + search: '?ctSummary=js-allocations', + }); + expect(getCallTreeSummaryStrategy(getState())).toEqual('js-allocations'); + }); + + it('can be native allocations', function() { + const { getState } = _getStoreWithURL({ + search: '?ctSummary=native-allocations', + }); + expect(getCallTreeSummaryStrategy(getState())).toEqual( + 'native-allocations' + ); + }); + + it('will use the default "timing" when an unknown value is received', function() { + const { getState } = _getStoreWithURL({ + search: '?ctSummary=unknown-value', + }); + expect(getCallTreeSummaryStrategy(getState())).toEqual('timing'); + }); +}); diff --git a/src/types/actions.js b/src/types/actions.js index 3877127136..2e7f552813 100644 --- a/src/types/actions.js +++ b/src/types/actions.js @@ -82,6 +82,11 @@ export type RequestedLib = {| +breakpadId: string, |}; export type ImplementationFilter = 'combined' | 'js' | 'cpp'; +// Change the strategy for computing the summarizing information for the call tree. +export type CallTreeSummaryStrategy = + | 'timing' + | 'js-allocations' + | 'native-allocations'; /** * This type determines what kind of information gets sanitized from published profiles. @@ -306,6 +311,10 @@ type UrlStateAction = +previousImplementation: ImplementationFilter, +implementation: ImplementationFilter, |} + | {| + type: 'CHANGE_CALL_TREE_SUMMARY_STRATEGY', + strategy: CallTreeSummaryStrategy, + |} | {| +type: 'CHANGE_INVERT_CALLSTACK', +invertCallstack: boolean, diff --git a/src/types/state.js b/src/types/state.js index 7f3b59013b..3fa53819a3 100644 --- a/src/types/state.js +++ b/src/types/state.js @@ -9,6 +9,7 @@ import type { DataSource, PreviewSelection, ImplementationFilter, + CallTreeSummaryStrategy, RequestedLib, TrackReference, TimelineType, @@ -173,6 +174,7 @@ export type UrlState = {| hiddenLocalTracksByPid: Map>, localTrackOrderByPid: Map, implementation: ImplementationFilter, + callTreeSummaryStrategy: CallTreeSummaryStrategy, invertCallstack: boolean, showJsTracerSummary: boolean, committedRanges: StartEndRange[], From 134eaac5ea93476016cdef7f555184b617143c00 Mon Sep 17 00:00:00 2001 From: Greg Tatum Date: Wed, 17 Jul 2019 09:37:30 -0500 Subject: [PATCH 2/2] Add better documentation for the call tree strategies --- src/actions/profile-view.js | 5 +++++ src/profile-logic/profile-data.js | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/actions/profile-view.js b/src/actions/profile-view.js index cf3fad48c7..4041e005cf 100644 --- a/src/actions/profile-view.js +++ b/src/actions/profile-view.js @@ -940,6 +940,11 @@ export function changeImplementationFilter( }; } +/** + * This action changes the strategy used to build and display the call tree. This could + * use sample data, or build a new call tree based off of allocation information stored + * in markers. + */ export function changeCallTreeSummaryStrategy( strategy: CallTreeSummaryStrategy ): Action { diff --git a/src/profile-logic/profile-data.js b/src/profile-logic/profile-data.js index 04a538deeb..542d776ee2 100644 --- a/src/profile-logic/profile-data.js +++ b/src/profile-logic/profile-data.js @@ -697,8 +697,11 @@ export function toValidCallTreeSummaryStrategy( case 'native-allocations': return strategy; default: - // Default to timing if the strategy is not recognized. This value can come + // Default to "timing" if the strategy is not recognized. This value can come // from a user-generated URL. + // e.g. `profiler.firefox.com/public/hash/ctSummary=tiiming` (note the typo.) + // This default branch will ensure we don't send values we don't understand to + // the store. return 'timing'; } }