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

Rewrite "React Queued Renders" panel for better perf, and make clear it's experimental #9775

Merged
merged 11 commits into from
Oct 12, 2023

Conversation

markerikson
Copy link
Collaborator

@markerikson markerikson commented Sep 28, 2023

This PR:

  • supersedes the original work in Analyze React+Redux perf timings using recording analysis #9410
    • splits the changes in that PR into two halves. The <ReactPanel> changes are in this PR. The <ReactReduxPerfPanel> changes will go in a follow-up stacked PR.
  • Scaffolds a new packages/replay-experimental folder by configuring TS and import sorting
  • Rewrites the existing <ReactPanel> to improve loading perf and rendering behavior:
    • Rewrites the API calls to use getPointStack instead of getAllFrames for faster stack trace data
    • Adds logic to format point stack frames with source information
    • Adds Suspense interval caches for hits to React internal methods like scheduleUpdateOnFiber
    • Uses a virtualized list to render the list items
  • Improves the existing logic for formatting function details:
    • Fully switches to using the sourcesById cache instead of Redux sources state
    • Handles formatting anonymous functions by looking at the parent function name if available (common with Redux thunks)
    • Copies support for formatting methods in classes such as render() from the backend routines logic
  • Moves both the <ReactPanel> component and all associated Suspense caches over to replay-experimental, to clarify that this logic is not production-ready yet

image

Future Work

There's a lot of things I've proved that I can do that are not in the current version of this PR:

  • Counting how many function components and class components rendered in a given render pass
  • Showing list items for React render commits in the React panel (ie, alternating calls to queue a render vs commits)
  • Showing the red "you are here" line in the list

@vercel
Copy link

vercel bot commented Sep 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
devtools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 12, 2023 9:08pm

@markerikson markerikson changed the title Rewrite "React Queued Renders" panel and make clear it's experimental Rewrite "React Queued Renders" panel for better perf, and make clear it's experimental Sep 28, 2023
@markerikson markerikson force-pushed the feature/FE-1631-react-perf-panel branch from ad43bc2 to 6b6a8ec Compare September 28, 2023 16:07
@replay-io
Copy link

replay-io bot commented Sep 28, 2023

E2E Tests

85 replays were recorded for 9a60782.

image 0 Failed
image 85 Passed

View test run on Replay ↗︎

Snapshot Tests

100 replays were recorded for 9a60782.

image 0 Failed
image 100 Passed

View test run on Replay ↗︎

@@ -187,6 +188,18 @@ export const sourcesByIdCache = createSingleEntryCache<
},
});

export const sourcesByPartialUrlCache: Cache<
Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of a nit: This cache feels a bit unnecessary. Seems like we could useMemo or in some other way cache the result of this filter action without adding a whole new cache for it. We're really only using it to look for a single file anyway (react-dom).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's being used for react-dom, but the call is inside of a couple different caches, and I've got a spot in the follow-up PR that I just noticed ought to be using this for Redux files as well.

updateMappedLocation(sources, functionLocation);

const location = getPreferredLocation(sourcesState, functionLocation);
// const { functionLocation } = fnPreview;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// const { functionLocation } = fnPreview;

import { useAppDispatch, useAppSelector } from "ui/setup/hooks";

import cardsListStyles from "ui/components/Comments/CommentCardsList.module.css";
import styles from "ui/components/Events/Event.module.css";
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems pretty fragile for this component to import styles from other packages. We don't have TypeScript checking us there so it seems we're likely to "break" the React styles by changes made in these other CSS files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. This was me trying to get something that looked passably decent, as quickly as possible (and an example of why I was saying we could start extracting some reusable components or styles).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should copy/fork the styles for now.

flex-direction: column;
padding: 0;
overflow: auto;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we delete this file? Seems like nothing is referencing it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It gets used in the follow-on PR - got accidentally included here as I was trying to split up the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Kill it from this PR?

src/ui/suspense/frameCache.ts Outdated Show resolved Hide resolved
src/ui/suspense/frameCache.ts Outdated Show resolved Hide resolved
src/ui/suspense/frameCache.ts Outdated Show resolved Hide resolved
@replay-delta
Copy link

replay-delta bot commented Oct 12, 2023

@markerikson markerikson force-pushed the feature/FE-1631-react-perf-panel branch from a2e31a6 to e94541d Compare October 12, 2023 20:50
@markerikson markerikson force-pushed the feature/FE-1631-react-perf-panel branch from e94541d to 9a60782 Compare October 12, 2023 21:05
@markerikson markerikson merged commit f914678 into main Oct 12, 2023
34 of 35 checks passed
@markerikson markerikson deleted the feature/FE-1631-react-perf-panel branch October 12, 2023 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants