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.

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

refactor[react-devtools/fiber/renderer]: optimize durations resolution #31118

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
24 changes: 19 additions & 5 deletions packages/react-devtools-shared/src/backend/fiber/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import {
} from 'react-devtools-shared/src/utils';
import {
formatConsoleArgumentsToSingleString,
formatDurationToMicrosecondsGranularity,
gt,
gte,
parseSourceFromComponentStack,
Expand Down Expand Up @@ -5074,20 +5075,33 @@ export function attach(
const fiberSelfDurations: Array<[number, number]> = [];
for (let i = 0; i < durations.length; i += 3) {
const fiberID = durations[i];
fiberActualDurations.push([fiberID, durations[i + 1]]);
fiberSelfDurations.push([fiberID, durations[i + 2]]);
fiberActualDurations.push([
fiberID,
formatDurationToMicrosecondsGranularity(durations[i + 1]),
]);
fiberSelfDurations.push([
fiberID,
formatDurationToMicrosecondsGranularity(durations[i + 2]),
]);
}

commitData.push({
changeDescriptions:
changeDescriptions !== null
? Array.from(changeDescriptions.entries())
: null,
duration: maxActualDuration,
effectDuration,
duration:
formatDurationToMicrosecondsGranularity(maxActualDuration),
effectDuration:
effectDuration !== null
? formatDurationToMicrosecondsGranularity(effectDuration)
: null,
fiberActualDurations,
fiberSelfDurations,
passiveEffectDuration,
passiveEffectDuration:
passiveEffectDuration !== null
? formatDurationToMicrosecondsGranularity(passiveEffectDuration)
: null,
Comment on lines +5093 to +5104
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be cleaner to have formatDurationToMicrosecondsGranularity take null? (I don't have a preference)

priorityLevel,
timestamp: commitTime,
updaters,
Expand Down
9 changes: 9 additions & 0 deletions packages/react-devtools-shared/src/backend/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -331,3 +331,12 @@ export function parseSourceFromComponentStack(

return parseSourceFromFirefoxStack(componentStack);
}

// 0.123456789 => 0.123
// Expects high-resolution timestamp in milliseconds, like from performance.now()
// Mainly used for optimizing the size of serialized profiling payload
export function formatDurationToMicrosecondsGranularity(
duration: number,
): number {
return Math.round(duration * 1000) / 1000;
}
Comment on lines +334 to +342
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be a risk of the frontend precision being out of sync with this here? I'd imagine an all-in solution involving the frontend declaring a precision somehow. (not saying we need it in this PR tho)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, yes, if we decide to start showing durations with increased granularity. This is not the case for browser extension, though, because backend and frontend are shipped in a single version lockstep.

52 changes: 38 additions & 14 deletions packages/react-devtools-shared/src/devtools/ProfilerStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import EventEmitter from '../events';
import {prepareProfilingDataFrontendFromBackendAndStore} from './views/Profiler/utils';
import ProfilingCache from './ProfilingCache';
import Store from './store';
import {logEvent} from 'react-devtools-shared/src/Logger';

import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
import type {ProfilingDataBackend} from 'react-devtools-shared/src/backend/types';
Expand Down Expand Up @@ -67,7 +68,12 @@ export default class ProfilerStore extends EventEmitter<{

// The backend is currently profiling.
// When profiling is in progress, operations are stored so that we can later reconstruct past commit trees.
_isProfiling: boolean = false;
_isBackendProfiling: boolean = false;

// Mainly used for optimistic UI.
// This could be false, but at the same time _isBackendProfiling could be true
// for cases when Backend is busy serializing a chunky payload.
_isProfilingBasedOnUserInput: boolean = false;

// Tracks whether a specific renderer logged any profiling data during the most recent session.
_rendererIDsThatReportedProfilingData: Set<number> = new Set();
Expand All @@ -86,7 +92,8 @@ export default class ProfilerStore extends EventEmitter<{
super();

this._bridge = bridge;
this._isProfiling = defaultIsProfiling;
this._isBackendProfiling = defaultIsProfiling;
this._isProfilingBasedOnUserInput = defaultIsProfiling;
this._store = store;

bridge.addListener('operations', this.onBridgeOperations);
Expand Down Expand Up @@ -139,8 +146,8 @@ export default class ProfilerStore extends EventEmitter<{
return this._rendererQueue.size > 0 || this._dataBackends.length > 0;
}

get isProfiling(): boolean {
return this._isProfiling;
get isProfilingBasedOnUserInput(): boolean {
return this._isProfilingBasedOnUserInput;
}

get profilingCache(): ProfilingCache {
Expand All @@ -151,7 +158,7 @@ export default class ProfilerStore extends EventEmitter<{
return this._dataFrontend;
}
set profilingData(value: ProfilingDataFrontend | null): void {
if (this._isProfiling) {
if (this._isBackendProfiling) {
console.warn(
'Profiling data cannot be updated while profiling is in progress.',
);
Expand Down Expand Up @@ -186,6 +193,9 @@ export default class ProfilerStore extends EventEmitter<{
startProfiling(): void {
this._bridge.send('startProfiling', this._store.recordChangeDescriptions);

this._isProfilingBasedOnUserInput = true;
this.emit('isProfiling');

// Don't actually update the local profiling boolean yet!
// Wait for onProfilingStatus() to confirm the status has changed.
// This ensures the frontend and backend are in sync wrt which commits were profiled.
Expand All @@ -195,8 +205,12 @@ export default class ProfilerStore extends EventEmitter<{
stopProfiling(): void {
this._bridge.send('stopProfiling');

// Don't actually update the local profiling boolean yet!
// Wait for onProfilingStatus() to confirm the status has changed.
// Backend might be busy serializing the payload, so we are going to display
// optimistic UI to the user that profiling is stopping.
this._isProfilingBasedOnUserInput = false;
this.emit('isProfiling');

// Wait for onProfilingStatus() to confirm the status has changed, this will update _isBackendProfiling.
// This ensures the frontend and backend are in sync wrt which commits were profiled.
// We do this to avoid mismatches on e.g. CommitTreeBuilder that would cause errors.
}
Expand Down Expand Up @@ -229,7 +243,7 @@ export default class ProfilerStore extends EventEmitter<{
const rendererID = operations[0];
const rootID = operations[1];

if (this._isProfiling) {
if (this._isBackendProfiling) {
let profilingOperations = this._inProgressOperationsByRootID.get(rootID);
if (profilingOperations == null) {
profilingOperations = [operations];
Expand All @@ -252,8 +266,8 @@ export default class ProfilerStore extends EventEmitter<{

onBridgeProfilingData: (dataBackend: ProfilingDataBackend) => void =
dataBackend => {
if (this._isProfiling) {
// This should never happen, but if it does- ignore previous profiling data.
if (this._isBackendProfiling) {
// This should never happen, but if it does, then ignore previous profiling data.
return;
}

Expand Down Expand Up @@ -289,7 +303,7 @@ export default class ProfilerStore extends EventEmitter<{
};

onProfilingStatus: (isProfiling: boolean) => void = isProfiling => {
if (this._isProfiling === isProfiling) {
if (this._isBackendProfiling === isProfiling) {
return;
}

Expand Down Expand Up @@ -319,15 +333,25 @@ export default class ProfilerStore extends EventEmitter<{
});
}

this._isProfiling = isProfiling;
this._isBackendProfiling = isProfiling;
// _isProfilingBasedOnUserInput should already be updated from startProfiling, stopProfiling, or constructor.
if (this._isProfilingBasedOnUserInput !== isProfiling) {
logEvent({
event_name: 'error',
error_message: `Unexpected profiling status. Expected ${this._isProfilingBasedOnUserInput.toString()}, but received ${isProfiling.toString()}.`,
error_stack: new Error().stack,
error_component_stack: null,
});

// If happened, fallback to displaying the value from Backend
this._isProfilingBasedOnUserInput = isProfiling;
}

// Invalidate suspense cache if profiling data is being (re-)recorded.
// Note that we clear again, in case any views read from the cache while profiling.
// (That would have resolved a now-stale value without any profiling data.)
this._cache.invalidate();

this.emit('isProfiling');

// If we've just finished a profiling session, we need to fetch data stored in each renderer interface
// and re-assemble it on the front-end into a format (ProfilingDataFrontend) that can power the Profiler UI.
// During this time, DevTools UI should probably not be interactive.
Expand Down
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/devtools/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ export default class Store extends EventEmitter<{
return this._componentFilters;
}
set componentFilters(value: Array<ComponentFilter>): void {
if (this._profilerStore.isProfiling) {
if (this._profilerStore.isProfilingBasedOnUserInput) {
// Re-mounting a tree while profiling is in progress might break a lot of assumptions.
// If necessary, we could support this- but it doesn't seem like a necessary use case.
this._throwAndEmitError(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ function ProfilerContextController({children}: Props): React.Node {
getCurrentValue: () => ({
didRecordCommits: profilerStore.didRecordCommits,
isProcessingData: profilerStore.isProcessingData,
isProfiling: profilerStore.isProfiling,
isProfiling: profilerStore.isProfilingBasedOnUserInput,
profilingData: profilerStore.profilingData,
supportsProfiling: store.rootSupportsBasicProfiling,
}),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export default function SettingsModal(): React.Node {
// Explicitly disallow it for now.
const isProfilingSubscription = useMemo(
() => ({
getCurrentValue: () => profilerStore.isProfiling,
getCurrentValue: () => profilerStore.isProfilingBasedOnUserInput,
subscribe: (callback: Function) => {
profilerStore.addListener('isProfiling', callback);
return () => profilerStore.removeListener('isProfiling', callback);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export default function SettingsModalContextToggle(): React.Node {
// Explicitly disallow it for now.
const isProfilingSubscription = useMemo(
() => ({
getCurrentValue: () => profilerStore.isProfiling,
getCurrentValue: () => profilerStore.isProfilingBasedOnUserInput,
subscribe: (callback: Function) => {
profilerStore.addListener('isProfiling', callback);
return () => profilerStore.removeListener('isProfiling', callback);
Expand Down
Loading