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

DevTools: Profiler refactor incremental changes #23185

Merged
merged 9 commits into from
Jan 28, 2022

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jan 26, 2022

Builds on top of PR #23158 and adds the following commits:

Commit Needs review? Description
35fb460 no Shifted code around to add "TODO" comments.
54a2675 yes Merged legacy and timeline so both can be viewed at the same time
9128307 no Updated test snapshots
0c180ab maybe Add new tests for in memory timeline data and export/import
3721c9b yes Fixed conditional wrappers
a64c4a6 yes Small bug fix discovered while testing

Related to #22529

Compare to the squashed devtools-timeline-profiler-squashed branch


Note that this PR fixes the Timeline profiler feature so that it's usable again. It should be safe to merge into main.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 26, 2022
@bvaughn bvaughn force-pushed the devtools-profiler-refactor-phase-6 branch 9 times, most recently from b1cb43b to 1571c58 Compare January 27, 2022 00:03
@sizebot
Copy link

sizebot commented Jan 27, 2022

Comparing: 2ed58eb...1154a73

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 129.59 kB 129.59 kB = 41.55 kB 41.55 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 134.77 kB 134.77 kB = 43.10 kB 43.10 kB
facebook-www/ReactDOM-prod.classic.js = 428.19 kB 428.19 kB = 78.60 kB 78.60 kB
facebook-www/ReactDOM-prod.modern.js = 418.18 kB 418.18 kB = 77.17 kB 77.17 kB
facebook-www/ReactDOMForked-prod.classic.js = 428.19 kB 428.19 kB = 78.60 kB 78.60 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 1154a73

@bvaughn bvaughn force-pushed the devtools-profiler-refactor-phase-6 branch 2 times, most recently from 154e92e to 19cbce1 Compare January 27, 2022 15:24
@bvaughn bvaughn force-pushed the devtools-profiler-refactor-phase-6 branch from 19cbce1 to 0c180ab Compare January 27, 2022 15:27
@bvaughn bvaughn requested a review from lunaruan January 27, 2022 16:00
@bvaughn bvaughn marked this pull request as ready for review January 27, 2022 17:35
@bvaughn bvaughn mentioned this pull request Jan 27, 2022
12 tasks
internalModuleSourceToRanges: Array<
[string, Array<[ErrorStackFrame, ErrorStackFrame]>],
>,
laneToLabelMap: Array<[ReactLane, string]>,
Copy link
Contributor

Choose a reason for hiding this comment

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

laneToLabelMap: LaneToLabelMap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maps don't serialize well. (They get converted to an empty object.) So we convert this to an array of key-value, as we do with the other maps in this data. Maybe I should rename the variable? The type is right though.

@@ -701,24 +684,7 @@ function processTimelineEvent(
currentProfilerData,
state.measureStack,
);
} else if (name.startsWith('--render-cancel')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why you deleted this and `--suspense-resuspend-'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed they weren't actually being used/logged anywhere.

packages/react-devtools-shell/src/app/index.js Outdated Show resolved Hide resolved
packages/react-devtools-shell/src/app/index.js Outdated Show resolved Hide resolved
scripts/jest/config.build-devtools.js Show resolved Hide resolved
const getCurrentTime =
typeof performance === 'object' && typeof performance.now === 'function'
? () => performance.now()
: () => Date.now();
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious how accurate this is in getting the current time (also thinking about this for the transition tracing stuff) and if it matters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this is what you're asking but we stub out the timer for DevTools tests so that it reads from unstable_now (in scheduler/src/forks/SchedulerMock) so that tests are repeatable.

@bvaughn bvaughn merged commit fa816be into facebook:main Jan 28, 2022
@bvaughn bvaughn deleted the devtools-profiler-refactor-phase-6 branch January 28, 2022 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Component: Developer Tools React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants