Skip to content

Commit

Permalink
fix[react-devtools]: update profiling status before receiving respons…
Browse files Browse the repository at this point in the history
…e from backend (#31117)

We can't wait for a response from Backend, because it might take some
time to actually finish profiling.

We should keep a flag on the frontend side, so user can quickly see the
feedback in the UI.
  • Loading branch information
hoxyq authored Oct 9, 2024
1 parent bf0c054 commit dbf80c8
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 18 deletions.
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

0 comments on commit dbf80c8

Please sign in to comment.