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
6 changes: 3 additions & 3 deletions src/profile-logic/marker-data.js
Original file line number Diff line number Diff line change
Expand Up @@ -496,9 +496,9 @@ export function deriveMarkersFromRawMarkerTable(
// In the case of separate markers for the start and end of an interval,
// merge the payloads together, with the end data overriding the start.
function mergeIntervalData(
startData: MarkerPayload,
endData: MarkerPayload
): MarkerPayload {
startData: MarkerPayload | null,
endData: MarkerPayload | null
): MarkerPayload | null {
if (!startData && !endData) {
return null;
}
Expand Down
27 changes: 17 additions & 10 deletions src/profile-logic/process-profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -661,13 +661,16 @@ function _processStackTable(
* synchronous stack. Otherwise, if it happened before, it was an async stack, and is
* most likely some event that happened in the past that triggered the marker.
*/
function _convertStackToCause(data: any): any {
function _convertStackToCause(data: MarkerPayload_Gecko) {
if ('stack' in data && data.stack && data.stack.samples.data.length > 0) {
const { stack, ...newData } = data;
const stackIndex = stack.samples.data[0][stack.samples.schema.stack];
const time = stack.samples.data[0][stack.samples.schema.time];
if (stackIndex !== null) {
newData.cause = { tid: stack.tid, time, stack: stackIndex };
return {
...newData,
cause: { tid: stack.tid, time, stack: stackIndex },
};
Comment on lines +670 to +673

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.

Theorically this might cause some performance regression, but I think this is tiny compared to the whole processing. We can revisit if we find this triggers problems.

}
return newData;
}
Expand All @@ -679,7 +682,7 @@ function _convertStackToCause(data: any): any {
* from a gecko payload.
*/
function _convertPayloadStackToIndex(
data: MarkerPayload_Gecko
data: MarkerPayload_Gecko | null
): IndexIntoStackTable | null {
if (!data) {
return null;
Expand Down Expand Up @@ -714,7 +717,7 @@ function _processMarkers(geckoMarkers: GeckoMarkerStruct): {|
let hasMemoryAddresses;

for (let markerIndex = 0; markerIndex < geckoMarkers.length; markerIndex++) {
const geckoPayload: MarkerPayload_Gecko = geckoMarkers.data[markerIndex];
const geckoPayload = geckoMarkers.data[markerIndex];
Comment on lines 717 to +720

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.

I found that this was quite obvious, but happy to revert if you prefer.


if (geckoPayload) {
switch (geckoPayload.type) {
Expand Down Expand Up @@ -844,8 +847,8 @@ function convertPhaseTimes(
* the GC information.
*/
function _processMarkerPayload(
geckoPayload: MarkerPayload_Gecko
): MarkerPayload {
geckoPayload: MarkerPayload_Gecko | null
): MarkerPayload | null {
if (!geckoPayload) {
return null;
}
Expand Down Expand Up @@ -904,10 +907,14 @@ function _processMarkerPayload(
}
}
default:
// Coerce the payload into a MarkerPayload. This doesn't really provide
// any more type safety, but it shows the intent of going from an object
// without much type safety, to a specific type definition.
return (payload: MarkerPayload);
// `payload` is currently typed as the result of _convertStackToCause, which
// is MarkerPayload_Gecko where `stack` has been replaced with `cause`. This
// should be reasonably close to `MarkerPayload`, but Flow doesn't work well
// with our MarkerPayload type. So we're coerce this return value to `any`
// here, and then to `MarkerPayload` as the return value for this function.
// This doesn't provide type safety but it shows the intent of going from an
// object without much type safety, to a specific type definition.
return (payload: any);

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 return type for this function is still MarkerPayload | null so we don't lost the type safety by using any here.

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.

Hmm, but do you know why we can't write MarkerPayload here anymore? It looks a bit suspicious. Is it because the payload is not any anymore? (also see my comment about why that is not correct)

Also a nit: I think we need to update the comment above of this as we are not coercing it to MarkerPayload anymore. But I think we will be able to coerce it again after making the payload any again.

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.

Is it because the payload is not any anymore? (also see my comment about why that is not correct)

Yep, exactly. But because the function return value is MarkerPayload it will still be coerced to MarkerPayload for the users of this function in the end. I can be more explicit here and coerce to MarkerPayload explicitely if you prefer.

}
}

Expand Down
4 changes: 2 additions & 2 deletions src/profile-logic/profile-data.js
Original file line number Diff line number Diff line change
Expand Up @@ -3091,8 +3091,8 @@ export function replaceStackReferences(

// Markers
function replaceStackReferenceInMarkerPayload(
oldData: MarkerPayload
): MarkerPayload {
oldData: MarkerPayload | null
): MarkerPayload | null {
if (oldData && 'cause' in oldData && oldData.cause) {
// Replace the cause with the right stack index.
// Use (...: any) because our current version of Flow has trouble with
Expand Down
2 changes: 1 addition & 1 deletion src/test/fixtures/profiles/gecko-profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ type TestDefinedGeckoMarker = {|
+endTime: Milliseconds | null,
+phase: MarkerPhase,
+category?: IndexIntoCategoryList,
+data?: MarkerPayload_Gecko,
+data?: MarkerPayload_Gecko | null,
|};

function _createGeckoThreadWithMarkers(
Expand Down
6 changes: 3 additions & 3 deletions src/test/fixtures/profiles/processed-profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export type TestDefinedMarkers = Array<
MarkerName,
MarkerTime, // start time
MarkerTime | null, // end time
MarkerPayload | MockPayload
MarkerPayload | MockPayload | null
]
>;

Expand All @@ -86,7 +86,7 @@ export type TestDefinedRawMarker = {|
+endTime: Milliseconds | null,
+phase: MarkerPhase,
+category?: IndexIntoCategoryList,
+data?: MarkerPayload,
+data?: MarkerPayload | null,
|};

export type TestDefinedJsTracerEvent = [
Expand Down Expand Up @@ -131,7 +131,7 @@ export function addMarkersToThreadWithCorrespondingSamples(
const startTime = tuple[1];
// Flow doesn't support variadic tuple types.
const maybeEndTime = (tuple: any)[2] || null;
const payload: MarkerPayload = (tuple: any)[3] || null;
const payload: MarkerPayload | null = (tuple: any)[3] || null;

markersTable.name.push(stringTable.indexForString(name));
if (maybeEndTime === null) {
Expand Down
4 changes: 2 additions & 2 deletions src/types/gecko-profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export type GeckoMarkerTuple = [
Milliseconds | null,
MarkerPhase,
IndexIntoCategoryList,
MarkerPayload_Gecko
MarkerPayload_Gecko | null
];

export type GeckoMarkers = {
Expand All @@ -70,7 +70,7 @@ export type GeckoMarkerStruct = {|
startTime: Milliseconds[],
endTime: Milliseconds[],
phase: MarkerPhase[],
data: MarkerPayload_Gecko[],
data: Array<MarkerPayload_Gecko | null>,
category: IndexIntoCategoryList[],
length: number,
|};
Expand Down
7 changes: 2 additions & 5 deletions src/types/markers.js
Original file line number Diff line number Diff line change
Expand Up @@ -766,8 +766,7 @@ export type MarkerPayload =
| MediaSampleMarkerPayload
| JankPayload
| BrowsertimeMarkerPayload
| NoPayloadUserData
| null;
| NoPayloadUserData;

export type MarkerPayload_Gecko =
| GPUMarkerPayload
Expand Down Expand Up @@ -795,6 +794,4 @@ export type MarkerPayload_Gecko =
| $ReplaceCauseWithStack<FileIoPayload>
| $ReplaceCauseWithStack<PaintProfilerMarkerTracing>
| $ReplaceCauseWithStack<StyleMarkerPayload>
| $ReplaceCauseWithStack<TextMarkerPayload>
// Payloads can be null.
| null;
| $ReplaceCauseWithStack<TextMarkerPayload>;
2 changes: 1 addition & 1 deletion src/types/profile-derived.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ export type Marker = {|
name: string,
category: IndexIntoCategoryList,
threadId: Tid | null,
data: MarkerPayload,
data: MarkerPayload | null,
incomplete?: boolean,
|};

Expand Down
2 changes: 1 addition & 1 deletion src/types/profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ export type ProfilerMarkerPayload = {
* it into a structured marker.
*/
export type RawMarkerTable = {|
data: MarkerPayload[],
data: Array<MarkerPayload | null>,
name: IndexIntoStringTable[],
startTime: Array<number | null>,
endTime: Array<number | null>,
Expand Down