Skip to content

Profiler integration with interaction-tracking package #13253

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.

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

Merged
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
1 change: 1 addition & 0 deletions fixtures/unstable-async/suspense/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"dependencies": {
"clipboard": "^1.7.1",
"github-fork-ribbon-css": "^0.2.1",
"interaction-tracking": "../../../build/node_modules/interaction-tracking",
"react": "../../../build/node_modules/react",
"react-dom": "../../../build/node_modules/react-dom",
"react-draggable": "^3.0.5",
Expand Down
33 changes: 22 additions & 11 deletions fixtures/unstable-async/suspense/src/components/App.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {track, wrap} from 'interaction-tracking';
import React, {Placeholder, PureComponent} from 'react';
import {createResource} from 'simple-cache-provider';
import {cache} from '../cache';
Expand Down Expand Up @@ -27,21 +28,31 @@ export default class App extends PureComponent {
}

handleUserClick = id => {
this.setState({
currentId: id,
});
requestIdleCallback(() => {
this.setState({
showDetail: true,
});
track(`View ${id}`, performance.now(), () => {
track(`View ${id} (high-pri)`, performance.now(), () =>
this.setState({
currentId: id,
})
);
requestIdleCallback(
wrap(() =>
track(`View ${id} (low-pri)`, performance.now(), () =>
this.setState({
showDetail: true,
})
)
)
);
});
};

handleBackClick = () =>
this.setState({
currentId: null,
showDetail: false,
});
track('View list', performance.now(), () =>
this.setState({
currentId: null,
showDetail: false,
})
);

render() {
const {currentId, showDetail} = this.state;
Expand Down
13 changes: 8 additions & 5 deletions fixtures/unstable-async/suspense/src/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {track} from 'interaction-tracking';
import React, {Fragment, PureComponent} from 'react';
import {unstable_createRoot, render} from 'react-dom';
import {cache} from './cache';
Expand Down Expand Up @@ -64,11 +65,13 @@ class Debugger extends PureComponent {
}

handleReset = () => {
cache.invalidate();
this.setState(state => ({
requests: {},
}));
handleReset();
track('Clear cache', () => {
cache.invalidate();
this.setState(state => ({
requests: {},
}));
handleReset();
});
};

handleProgress = (url, progress, isPaused) => {
Expand Down
3 changes: 3 additions & 0 deletions fixtures/unstable-async/suspense/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3482,6 +3482,9 @@ inquirer@3.3.0, inquirer@^3.0.6:
strip-ansi "^4.0.0"
through "^2.3.6"

interaction-tracking@../../../build/node_modules/interaction-tracking:
version "0.0.1"

internal-ip@1.2.0:
version "1.2.0"
resolved "https://registry.yarnpkg.com/internal-ip/-/internal-ip-1.2.0.tgz#ae9fbf93b984878785d50a8de1b356956058cf5c"
Expand Down
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type {UpdateQueue} from './ReactUpdateQueue';
import type {ContextDependency} from './ReactFiberNewContext';

import invariant from 'shared/invariant';
import warningWithoutStack from 'shared/warningWithoutStack';
import {enableProfilerTimer} from 'shared/ReactFeatureFlags';
import {NoEffect} from 'shared/ReactSideEffectTags';
import {
Expand Down Expand Up @@ -531,7 +532,7 @@ export function createFiberFromProfiler(
typeof pendingProps.id !== 'string' ||
typeof pendingProps.onRender !== 'function'
) {
invariant(
warningWithoutStack(
false,
'Profiler must specify an "id" string and "onRender" function as props',
);
Expand Down
41 changes: 31 additions & 10 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ import type {FiberRoot} from './ReactFiberRoot';
import type {ExpirationTime} from './ReactFiberExpirationTime';
import type {CapturedValue, CapturedError} from './ReactCapturedValue';

import {enableProfilerTimer, enableSuspense} from 'shared/ReactFeatureFlags';
import {
enableInteractionTracking,
enableProfilerTimer,
enableSuspense,
} from 'shared/ReactFeatureFlags';
import {
ClassComponent,
ClassComponentLazy,
Expand Down Expand Up @@ -777,7 +781,11 @@ function commitDeletion(current: Fiber): void {
detachFiber(current);
}

function commitWork(current: Fiber | null, finishedWork: Fiber): void {
function commitWork(
root: FiberRoot,
current: Fiber | null,
finishedWork: Fiber,
): void {
if (!supportsMutation) {
commitContainer(finishedWork);
return;
Expand Down Expand Up @@ -836,14 +844,27 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
case Profiler: {
if (enableProfilerTimer) {
const onRender = finishedWork.memoizedProps.onRender;
onRender(
finishedWork.memoizedProps.id,
current === null ? 'mount' : 'update',
finishedWork.actualDuration,
finishedWork.treeBaseDuration,
finishedWork.actualStartTime,
getCommitTime(),
);

if (enableInteractionTracking) {
onRender(
finishedWork.memoizedProps.id,
current === null ? 'mount' : 'update',
finishedWork.actualDuration,
finishedWork.treeBaseDuration,
finishedWork.actualStartTime,
getCommitTime(),
root.memoizedInteractions,
);
} else {
onRender(
finishedWork.memoizedProps.id,
current === null ? 'mount' : 'update',
finishedWork.actualDuration,
finishedWork.treeBaseDuration,
finishedWork.actualStartTime,
getCommitTime(),
);
}
}
return;
}
Expand Down
117 changes: 90 additions & 27 deletions packages/react-reconciler/src/ReactFiberRoot.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@
import type {Fiber} from './ReactFiber';
import type {ExpirationTime} from './ReactFiberExpirationTime';
import type {TimeoutHandle, NoTimeout} from './ReactFiberHostConfig';
import type {Interaction} from 'interaction-tracking/src/InteractionTracking';

import {noTimeout} from './ReactFiberHostConfig';

import {createHostRootFiber} from './ReactFiber';
import {NoWork} from './ReactFiberExpirationTime';
import {enableInteractionTracking} from 'shared/ReactFeatureFlags';
import {getThreadID} from 'interaction-tracking';

// TODO: This should be lifted into the renderer.
export type Batch = {
Expand All @@ -24,7 +26,9 @@ export type Batch = {
_next: Batch | null,
};

export type FiberRoot = {
export type PendingInteractionMap = Map<ExpirationTime, Set<Interaction>>;

type BaseFiberRootProperties = {|
// Any additional information from the host associated with this root.
containerInfo: any,
// Used only by persistent updates.
Expand Down Expand Up @@ -73,6 +77,26 @@ export type FiberRoot = {
firstBatch: Batch | null,
// Linked-list of roots
nextScheduledRoot: FiberRoot | null,
|};

// The following attributes are only used by interaction tracking builds.
// They enable interactions to be associated with their async work,
// And expose interaction metadata to the React DevTools Profiler plugin.
// Note that these attributes are only defined when the enableInteractionTracking flag is enabled.
type ProfilingOnlyFiberRootProperties = {|
interactionThreadID: number,
memoizedInteractions: Set<Interaction>,
pendingInteractionMap: PendingInteractionMap,
|};

// Exported FiberRoot type includes all properties,
// To avoid requiring potentially error-prone :any casts throughout the project.
// Profiling properties are only safe to access in profiling builds (when enableInteractionTracking is true).
// The types are defined separately within this file to ensure they stay in sync.
// (We don't have to use an inline :any cast when enableInteractionTracking is disabled.)
export type FiberRoot = {
...BaseFiberRootProperties,
...ProfilingOnlyFiberRootProperties,
};

export function createFiberRoot(
Expand All @@ -83,30 +107,69 @@ export function createFiberRoot(
// Cyclic construction. This cheats the type system right now because
// stateNode is any.
const uninitializedFiber = createHostRootFiber(isAsync);
const root = {
current: uninitializedFiber,
containerInfo: containerInfo,
pendingChildren: null,

earliestPendingTime: NoWork,
latestPendingTime: NoWork,
earliestSuspendedTime: NoWork,
latestSuspendedTime: NoWork,
latestPingedTime: NoWork,

didError: false,

pendingCommitExpirationTime: NoWork,
finishedWork: null,
timeoutHandle: noTimeout,
context: null,
pendingContext: null,
hydrate,
nextExpirationTimeToWorkOn: NoWork,
expirationTime: NoWork,
firstBatch: null,
nextScheduledRoot: null,
};

let root;
if (enableInteractionTracking) {
root = ({
current: uninitializedFiber,
containerInfo: containerInfo,
pendingChildren: null,

earliestPendingTime: NoWork,
latestPendingTime: NoWork,
earliestSuspendedTime: NoWork,
latestSuspendedTime: NoWork,
latestPingedTime: NoWork,

didError: false,

pendingCommitExpirationTime: NoWork,
finishedWork: null,
timeoutHandle: noTimeout,
context: null,
pendingContext: null,
hydrate,
nextExpirationTimeToWorkOn: NoWork,
expirationTime: NoWork,
firstBatch: null,
nextScheduledRoot: null,

interactionThreadID: getThreadID(),
memoizedInteractions: new Set(),
pendingInteractionMap: new Map(),
}: FiberRoot);
} else {
root = ({
current: uninitializedFiber,
containerInfo: containerInfo,
pendingChildren: null,

earliestPendingTime: NoWork,
latestPendingTime: NoWork,
earliestSuspendedTime: NoWork,
latestSuspendedTime: NoWork,
latestPingedTime: NoWork,

didError: false,

pendingCommitExpirationTime: NoWork,
finishedWork: null,
timeoutHandle: noTimeout,
context: null,
pendingContext: null,
hydrate,
nextExpirationTimeToWorkOn: NoWork,
expirationTime: NoWork,
firstBatch: null,
nextScheduledRoot: null,
}: BaseFiberRootProperties);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try using an intersection type here? Or a spread type. Then the any below shouldn't be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. This was the only type combo I could come up with that worked. If you can show me another syntax that Flow would support though, I'm happy to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW I added a $FlowFixMe comment here. I don't know of a better way to do this, but we can improve it with a follow up PR.

}

Copy link
Contributor

@NE-SmallTown NE-SmallTown Sep 1, 2018

Choose a reason for hiding this comment

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

@bvaughn Can I ask that why we do the repetitive logic, i.e. only interactionThreadID, memoizedInteractions and pendingInteractionMap is related to enableInteractionTracking, we don't need to copy the remaining fields here, why not just do something like if (enableInteractionTracking) { root.interactionThreadID === xxx} or if (enableInteractionTracking) { root === { ...root, interactionThreadID: xxx } }( or use Object.assign here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do it for performance since this code is really "hot" (it runs a lot). Seb would be a much better person to answer this question but I'll take a stab at it. 😄

JavaScript VM makes property lookup faster for objects by creating a class in the native-code layer based on the "shape" of the object (e.g. what keys it has). This is called the "hidden class", and it's a runtime optimization. If the VM isn't sure about the object's shape, it uses a slower (hash table) method to lookup properties. When fields are added/deleted after an object is created, it breaks the VM's "hidden class" assumptions and de-opts to hash table lookup.

Probably worth remembering that enableInteractionTracking is a compile time flag so this if/else won't exist in the built bundle. 😄

Copy link
Contributor

@NE-SmallTown NE-SmallTown Sep 1, 2018

Choose a reason for hiding this comment

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

uninitializedFiber.stateNode = root;
return root;

// The reason for the way the Flow types are structured in this file,
// Is to avoid needing :any casts everywhere interaction-tracking fields are used.
// Unfortunately that requires an :any cast for non-interaction-tracking capable builds.
// $FlowFixMe Remove this :any cast and replace it with something better.
return ((root: any): FiberRoot);
}
Loading