Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Optimize flamechart][1/4] Add flamechart to ReactProfilerData #90

Merged
merged 2 commits into from
Jul 28, 2020
Merged

[Optimize flamechart][1/4] Add flamechart to ReactProfilerData #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41fd630
Fix lint failure from #88
taneliang Jul 28, 2020
dbb59c9
[Optimize flamechart][1/4] Add flamechart to ReactProfilerData
taneliang Jul 28, 2020
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
23 changes: 5 additions & 18 deletions src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
// @flow

import type {FlamechartData, ReactProfilerData} from './types';
import type {ReactProfilerData} from './types';

import React, {useState, useCallback} from 'react';
import {unstable_batchedUpdates} from 'react-dom';
import React, {useState} from 'react';

import ImportPage from './ImportPage';
import CanvasPage from './CanvasPage';
Expand All @@ -12,22 +11,10 @@ export default function App() {
const [profilerData, setProfilerData] = useState<ReactProfilerData | null>(
null,
);
const [flamechart, setFlamechart] = useState<FlamechartData | null>(null);

const handleDataImported = useCallback(
(
importedProfilerData: ReactProfilerData,
importedFlamechart: FlamechartData,
) => {
unstable_batchedUpdates(() => {
setProfilerData(importedProfilerData);
setFlamechart(importedFlamechart);
});
},
);
if (profilerData && flamechart) {
return <CanvasPage profilerData={profilerData} flamechart={flamechart} />;
if (profilerData) {
return <CanvasPage profilerData={profilerData} />;
} else {
return <ImportPage onDataImported={handleDataImported} />;
return <ImportPage onDataImported={setProfilerData} />;
}
}
30 changes: 6 additions & 24 deletions src/CanvasPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,7 @@ import {ContextMenu, ContextMenuItem, useContextMenu} from './context';

const CONTEXT_MENU_ID = 'canvas';

import type {
FlamechartData,
ReactHoverContextInfo,
ReactProfilerData,
} from './types';
import type {ReactHoverContextInfo, ReactProfilerData} from './types';
import {useCanvasInteraction} from './useCanvasInteraction';
import {
FlamegraphView,
Expand All @@ -48,28 +44,21 @@ import {

type ContextMenuContextData = {|
data: ReactProfilerData,
flamechart: FlamechartData,
hoveredEvent: ReactHoverContextInfo | null,
|};

type Props = {|
profilerData: ReactProfilerData,
flamechart: FlamechartData,
|};

function CanvasPage({profilerData, flamechart}: Props) {
function CanvasPage({profilerData}: Props) {
return (
<div
className={styles.CanvasPage}
style={{backgroundColor: COLORS.BACKGROUND}}>
<AutoSizer>
{({height, width}: {height: number, width: number}) => (
<AutoSizedCanvas
data={profilerData}
flamechart={flamechart}
height={height}
width={width}
/>
<AutoSizedCanvas data={profilerData} height={height} width={width} />
)}
</AutoSizer>
</div>
Expand Down Expand Up @@ -104,17 +93,11 @@ const copySummary = (data, measure) => {

type AutoSizedCanvasProps = {|
data: ReactProfilerData,
flamechart: FlamechartData,
height: number,
width: number,
|};

function AutoSizedCanvas({
data,
flamechart,
height,
width,
}: AutoSizedCanvasProps) {
function AutoSizedCanvas({data, height, width}: AutoSizedCanvasProps) {
const canvasRef = useRef<HTMLCanvasElement | null>(null);

const [isContextMenuShown, setIsContextMenuShown] = useState<boolean>(false);
Expand Down Expand Up @@ -156,7 +139,7 @@ function AutoSizedCanvas({
const flamegraphView = new FlamegraphView(
surfaceRef.current,
{origin: zeroPoint, size: {width, height}},
flamechart,
data.flamechart,
data,
);
flamegraphViewRef.current = flamegraphView;
Expand Down Expand Up @@ -194,7 +177,7 @@ function AutoSizedCanvas({
);

surfaceRef.current.rootView = rootViewRef.current;
}, [data, flamechart, setHoveredEvent]);
}, [data, setHoveredEvent]);

useLayoutEffect(() => {
if (canvasRef.current) {
Expand Down Expand Up @@ -229,7 +212,6 @@ function AutoSizedCanvas({
useContextMenu<ContextMenuContextData>({
data: {
data,
flamechart,
hoveredEvent,
},
id: CONTEXT_MENU_ID,
Expand Down
20 changes: 5 additions & 15 deletions src/ImportPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,27 @@
// @flow

import type {TimelineEvent} from '@elg/speedscope';
import type {FlamechartData, ReactProfilerData} from './types';
import type {ReactProfilerData} from './types';

import React, {useEffect, useCallback, useRef} from 'react';
import profilerBrowser from './assets/profilerBrowser.png';
import style from './ImportPage.css';

import preprocessData from './util/preprocessData';
import preprocessFlamechart from './util/preprocessFlamechart';
import {readInputData} from './util/readInputData';

// TODO: Use for dev only, switch to import file after
import JSON_PATH from 'url:../static/perfprofilev2.json';

type Props = {|
onDataImported: (
profilerData: ReactProfilerData,
flamechart: FlamechartData,
) => void,
onDataImported: (profilerData: ReactProfilerData) => void,
|};

export default function ImportPage({onDataImported}: Props) {
const processTimeline = useCallback(
(events: TimelineEvent[]) => {
// Filter null entries and sort by timestamp.
// I would not expect to have to do either of this,
// but some of the data being passed in requires it.
events = events.filter(Boolean).sort((a, b) => (a.ts > b.ts ? 1 : -1));

if (events.length > 0) {
const processedData = preprocessData(events);
const processedFlamechart = preprocessFlamechart(events);
onDataImported(processedData, processedFlamechart);
onDataImported(preprocessData(events));
}
},
[onDataImported],
Expand Down Expand Up @@ -81,7 +70,8 @@ export default function ImportPage({onDataImported}: Props) {
</a>{' '}
from Chrome Devtools.
<br />
To zoom, scroll while holding down <kbd>Ctrl</kbd> or <kbd>Shift</kbd>
To zoom, scroll while holding down <kbd>Ctrl</kbd> or{' '}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is from #89's commit, that's also in this PR.

@kartikcho I'll be trying to rebase merge this PR, to see if #89's commit will appear twice on master (let's hope it doesn't). If #89's commit appears twice, I'll force push to the remaining PRs in this stack before merging them, to prevent polluting master.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: Only one commit appeared on master :D We're all good

<kbd>Shift</kbd>
<p className={style.legendKey}>
<svg height="20" width="20">
<circle cx="10" cy="10" r="5" fill="#ff718e" />
Expand Down
5 changes: 3 additions & 2 deletions src/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,11 +87,14 @@ export type ReactMeasure = {|
+depth: number,
|};

export type FlamechartData = Flamechart;

export type ReactProfilerData = {|
startTime: number,
duration: number,
events: ReactEvent[],
measures: ReactMeasure[],
flamechart: FlamechartData,
|};

export type ReactHoverContextInfo = {|
Expand All @@ -100,5 +103,3 @@ export type ReactHoverContextInfo = {|
data: $ReadOnly<ReactProfilerData> | null,
flamechartNode: FlamechartFrame | null,
|};

export type FlamechartData = Flamechart;
22 changes: 20 additions & 2 deletions src/util/preprocessData.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
// @flow

import {importFromChromeTimeline, Flamechart} from '@elg/speedscope';
import type {TimelineEvent} from '@elg/speedscope';
import type {
Milliseconds,
BatchUID,
FlamechartData,
ReactLane,
ReactMeasureType,
ReactProfilerData,
Expand Down Expand Up @@ -345,18 +347,34 @@ function processTimelineEvent(
}
}

function preprocessFlamechart(rawData: TimelineEvent[]): FlamechartData {
const parsedData = importFromChromeTimeline(rawData, 'react-devtools');
const profile = parsedData.profiles[0]; // TODO: Choose the main CPU thread only
const flamechart = new Flamechart({
getTotalWeight: profile.getTotalWeight.bind(profile),
forEachCall: profile.forEachCall.bind(profile),
formatValue: profile.formatValue.bind(profile),
getColorBucketForFrame: () => 0,
});
return flamechart;
}

export default function preprocessData(
timeline: TimelineEvent[],
): ReactProfilerData {
const profilerData = {
const flamechart = preprocessFlamechart(timeline);

Copy link
Member

Choose a reason for hiding this comment

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

Nit: Might wanna remove this extra line for no reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

This empty line was intentional as I want to separate the profiler data declaration that belongs to the rest of this function from the flamechart computation. Imo they're doing different things conceptually so they should be in separate "paragraphs"

const profilerData: ReactProfilerData = {
startTime: 0,
duration: 0,
events: [],
measures: [],
flamechart,
};

// TODO: Sort `timeline`. JSON Array Format trace events need not be ordered. See:
// Sort `timeline`. JSON Array Format trace events need not be ordered. See:
// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.f2f0yd51wi15
timeline = timeline.filter(Boolean).sort((a, b) => (a.ts > b.ts ? 1 : -1));

// Events displayed in flamechart have timestamps relative to the profile
// event's startTime. Source: https://github.com/v8/v8/blob/44bd8fd7/src/inspector/js_protocol.json#L1486
Expand Down
21 changes: 0 additions & 21 deletions src/util/preprocessFlamechart.js
View file
Open in desktop

This file was deleted.