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

chore: add dev:million-lint command #8032

Open
wants to merge 1 commit into
base: next
Choose a base branch
from
Open

Conversation

stipsan
Copy link
Member

@stipsan stipsan commented Dec 13, 2024

Description

This PR sets up a script that run Million Lint on the Test Studio locally:

million.mov

There's a lot to say on this topic, and if performance work interests you and you have a hot beverage ready, expand the Verbose information toggle at the end of the PR. The PR will otherwise be as brief and easy to review as possible 🙌

With Million Lint we can:

  • Find good ways to manually memoize components that React Compiler skips
    • The Compiler is in beta, so there are some React patterns that are valid, but not yet supported. Example
    • Once the Compiler is stable, there's still patterns it won't support. For example external state management like rxjs. And it'll always focus on correctness, which means it won't flag cases where components should make better use of composition to improve performance. It takes our code and tries its best to make it run as fast as possible, while preserving the original intent of life cycles like useEffect.
  • Record profile sessions that expose its data and metrics in the same way TypeScript flags errors, and ESLint shows warnings. You stay in the editor and iterate quickly and see the impact of changes immediately.
  • It can analyze general metrics that are always collected by simply browsing around and interacting with the Test Studio (you don't have to start the component profiler), and then uses an LLM to prioritize which components that could be optimized, based on how slow they are, their impact, and provides you with suggested fixes similar to GitHub Copilot.
  • In the future we can run it in production, similar to react production profiling, as opt-in, to analyze userland custom studios to look at real world performance bottlenecks that points us to exactly which component, which prop, which custom hook, that's causing a bottleneck.

What to review

Have a look on the diff, as I'm calling out what to review as my own review comments.
Please mark them as resolved once you've seen them, and feel free to ask direct follow up questions there and then resolve once you're satisifed with my answer 😌

Testing

Until Million supports running in production you'll only able to run it locally, and AFAIK the IDE extension is only available on VS Code for now.

  1. Install the extension
  2. git clone, pnpm install and run pnpm dev:million-lint.
  3. Open http://localhost:3333 and browse around.
  4. Open the Million extension in your IDE, you should now start seeing data coming in. You should also see inline data show up as you browse files that are instrumented.

You can look at dev/test-studio/.react-compiler-bailout-report.json if you wonder exactly what files are instrumented and will have intellisense available.

Why can't we just run Million on every file?

  1. Million doesn't yet support instrumenting files that are optimized by React Compiler. You'll see crashes like this.
  2. Due to our "death by a thousand cuts/renders" situation, instrumenting all our (over a thousand components) quickly lead to obscene amounts of data, eventually crashing your browser and VS Code (or slowing down your computer to a grinding halt, until you force quit them).
    Since we currently have to disable React Compiler if we want to run Million on everything, it significantly increases renders and thus render data.
    This isn't the first time we've had a problem like this. Our very own @runeb has official React contributor status after he sent a fix that allowed us to run the React DevTools profiler on more than just a few seconds in the Studio.
    On an M1 Pro 32GB RAM MacBook Pro you'll barely able to view a simple document list in Structure Tool, open a document, or type to fast in the search input and it dies.
    On a M4 Max 128GB RAM MacBook Pro you get quite a bit further, and can type into fields for a few minutes before it caves. Typing into a Portable Text field kills it almost instantly.

Notes for release

N/A this is purely a monorepo addition, only to the test studio. It doesn't affect what we publish to npm with sanity or @sanity/vision in any way.

Verbose information

How Million and React Scan compares to React DevTools

Million builds on top of React Scan, and integrates its performance profiling data with VS Code.
The benefit of React Scan, over the regular React Profiler in React DevTools, is that it instruments and traces how often hooks, props and state change over time and how it leads to cascading rerenders.
React DevTools can tell you what happened on each render frame, and is enough when you deal with performance issues where a single render is very slow.

In our codebase we have very few problems like that (they happen at scale, such as very large documents, large amount of fields, massive amount of editors working in the same document, huge Portable Text fields, those kind of scenarios). The general jank and slowness in the Studio is due to the "death of a thousand cuts". We have constant, cascading renders, paired with memoization that triggers garbage collection events as the memoization can't survive unstable props, state and context values. React DevTools isn't well equipped to help us here, what we need is the ability to track changes and counters over time in order to find the needles in our haystack.

How does React Compiler come into play?

Copy link

vercel bot commented Dec 13, 2024

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

Name Status Preview Comments Updated (UTC)
page-building-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 13, 2024 0:52am
performance-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 13, 2024 0:52am
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 13, 2024 0:52am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 13, 2024 0:52am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview Dec 13, 2024 0:52am

Copy link

socket-security bot commented Dec 13, 2024

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@million/[email protected] environment, eval, filesystem, network, shell Transitive: unsafe +121 64.8 MB nisargptel

View full report↗︎

@stipsan stipsan force-pushed the add-million-lint-mode branch from 30be234 to 7e5974a Compare December 13, 2024 10:38
Copy link
Contributor

No changes to documentation

Copy link
Contributor

github-actions bot commented Dec 13, 2024

⚡️ Editor Performance Report

Updated Fri, 13 Dec 2024 12:36:29 GMT

Benchmark reference
latency of sanity@latest
experiment
latency of this branch
Δ (%)
latency difference
article (title) 23.8 efps (42ms) 23.0 efps (44ms) +2ms (+3.6%)
article (body) 63.3 efps (16ms) 62.9 efps (16ms) +0ms (-/-%)
article (string inside object) 23.3 efps (43ms) 23.8 efps (42ms) -1ms (-2.3%)
article (string inside array) 20.8 efps (48ms) 21.3 efps (47ms) -1ms (-2.1%)
recipe (name) 52.6 efps (19ms) 52.6 efps (19ms) +0ms (-/-%)
recipe (description) 58.8 efps (17ms) 58.8 efps (17ms) +0ms (-/-%)
recipe (instructions) 99.9+ efps (5ms) 99.9+ efps (5ms) +0ms (-/-%)
synthetic (title) 19.6 efps (51ms) 19.6 efps (51ms) +0ms (-/-%)
synthetic (string inside object) 20.4 efps (49ms) 20.0 efps (50ms) +1ms (+2.0%)

efps — editor "frames per second". The number of updates assumed to be possible within a second.

Derived from input latency. efps = 1000 / input_latency

Detailed information

🏠 Reference result

The performance result of sanity@latest

Benchmark latency p75 p90 p99 blocking time test duration
article (title) 42ms 46ms 49ms 169ms 158ms 11.2s
article (body) 16ms 18ms 26ms 129ms 236ms 5.7s
article (string inside object) 43ms 45ms 48ms 177ms 143ms 7.3s
article (string inside array) 48ms 52ms 56ms 223ms 172ms 7.7s
recipe (name) 19ms 21ms 24ms 45ms 0ms 6.9s
recipe (description) 17ms 18ms 19ms 33ms 0ms 4.4s
recipe (instructions) 5ms 6ms 8ms 24ms 0ms 3.0s
synthetic (title) 51ms 54ms 59ms 284ms 501ms 13.9s
synthetic (string inside object) 49ms 53ms 60ms 260ms 491ms 8.3s

🧪 Experiment result

The performance result of this branch

Benchmark latency p75 p90 p99 blocking time test duration
article (title) 44ms 47ms 48ms 223ms 257ms 11.4s
article (body) 16ms 18ms 26ms 156ms 196ms 5.5s
article (string inside object) 42ms 45ms 47ms 146ms 11ms 6.9s
article (string inside array) 47ms 50ms 60ms 178ms 150ms 7.5s
recipe (name) 19ms 20ms 24ms 45ms 0ms 7.1s
recipe (description) 17ms 19ms 22ms 31ms 0ms 4.5s
recipe (instructions) 5ms 7ms 8ms 9ms 0ms 3.0s
synthetic (title) 51ms 54ms 57ms 173ms 395ms 17.8s
synthetic (string inside object) 50ms 54ms 64ms 246ms 485ms 8.2s

📚 Glossary

column definitions

  • benchmark — the name of the test, e.g. "article", followed by the label of the field being measured, e.g. "(title)".
  • latency — the time between when a key was pressed and when it was rendered. derived from a set of samples. the median (p50) is shown to show the most common latency.
  • p75 — the 75th percentile of the input latency in the test run. 75% of the sampled inputs in this benchmark were processed faster than this value. this provides insight into the upper range of typical performance.
  • p90 — the 90th percentile of the input latency in the test run. 90% of the sampled inputs were faster than this. this metric helps identify slower interactions that occurred less frequently during the benchmark.
  • p99 — the 99th percentile of the input latency in the test run. only 1% of sampled inputs were slower than this. this represents the worst-case scenarios encountered during the benchmark, useful for identifying potential performance outliers.
  • blocking time — the total time during which the main thread was blocked, preventing user input and UI updates. this metric helps identify performance bottlenecks that may cause the interface to feel unresponsive.
  • test duration — how long the test run took to complete.

Copy link
Contributor

github-actions bot commented Dec 13, 2024

Component Testing Report Updated Dec 13, 2024 12:34 PM (UTC)

❌ Failed Tests (4) -- expand for details
File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 1m 14s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 13s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 41s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 56s 11 7 0
formBuilder/inputs/PortableText/copyPaste/CopyPasteFields.spec.tsx ✅ Passed (Inspect) 0s 0 12 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 28s 6 0 0
formBuilder/inputs/PortableText/DisableFocusAndUnset.spec.tsx ✅ Passed (Inspect) 15s 3 0 0
formBuilder/inputs/PortableText/DragAndDrop.spec.tsx ❌ Failed (Inspect) 3m 8s 2 0 4
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 1m 13s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 3m 0s 21 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 49s 18 0 0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx ✅ Passed (Inspect) 14s 3 9 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 42s 9 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 29s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 1m 54s 21 0 0
formBuilder/tree-editing/TreeEditing.spec.tsx ✅ Passed (Inspect) 0s 0 3 0
formBuilder/tree-editing/TreeEditingNestedObjects.spec.tsx ✅ Passed (Inspect) 0s 0 3 0

@stipsan stipsan force-pushed the add-million-lint-mode branch from 7414464 to 9d075f4 Compare December 13, 2024 12:23
@stipsan stipsan marked this pull request as ready for review December 13, 2024 12:38
@stipsan stipsan requested a review from a team as a code owner December 13, 2024 12:38
@stipsan stipsan requested review from ryanbonial and removed request for a team December 13, 2024 12:38
@@ -83,3 +83,6 @@ yalc.lock
## Documentation Report
scripts/docs-report.md

# Temporary data collected by Million Lint
**/.million/store.json
dev/test-studio/.react-compiler-bailout-report.json
Copy link
Member Author

Choose a reason for hiding this comment

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

Committing this to git would cause all sorts of annoying merge conflicts on PRs that do perf work, that for example makes components no longer bail out. Or when eslint-plugin-react-compiler and babel-plugin-react-compiler itself changes what cases it bails and supports as we bump deps.

? {
target: '18',
sources: (filename) => {
if (filename.includes('node_modules')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the default behavior of sources, since we're overriding it we have to ensure we don't accidentally try running the compiler on non-src files from npm.

vite(viteConfig: UserConfig): UserConfig {
const reactProductionProfiling = process.env.REACT_PRODUCTION_PROFILING === 'true'

return {
...viteConfig,
plugins: millionLintEnabled
? [
require('@million/lint').vite({
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to use it as a regular import at the top originally.
But for some reason, if you have this line top-level:

import Million from '@million/lint'

then sanity build hangs forever

@@ -17,7 +17,8 @@
"check:deps": "pnpm --recursive --parallel exec depcheck",
"check:format": "prettier . --check",
"check:lint": "turbo run lint --continue -- --quiet",
"check:react-compiler": "eslint --no-inline-config --no-eslintrc --ext .cjs,.mjs,.js,.jsx,.ts,.tsx --parser @typescript-eslint/parser --plugin react-compiler --rule 'react-compiler/react-compiler: [warn]' --ignore-path .eslintignore.react-compiler --max-warnings 79 .",
"check:react-compiler": "eslint --cache --no-inline-config --no-eslintrc --ext .cjs,.mjs,.js,.jsx,.ts,.tsx --parser @typescript-eslint/parser --plugin react-compiler --rule 'react-compiler/react-compiler: [warn]' --ignore-path .eslintignore.react-compiler --max-warnings 77 .",
"report:react-compiler-bailout": "eslint --cache --no-inline-config --no-eslintrc --ext .cjs,.mjs,.js,.jsx,.ts,.tsx --parser @typescript-eslint/parser --plugin react-compiler --rule 'react-compiler/react-compiler: [error,{__unstable_donotuse_reportAllBailouts:true}]' --ignore-path .eslintignore.react-compiler -f ./scripts/reactCompilerBailouts.cjs . || true",
Copy link
Member Author

Choose a reason for hiding this comment

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

The reason this command has __unstable_donotuse_reportAllBailouts:true is because we want to make sure Million Lint runs on all components that are not optimized by the compiler.

Running the eslint compiler plugin reports 77 issues atm that it considers actionable.
Actionable means its bailing because of a problem with the code, for example reading a ref.current during render (which has always been unsafe, and the reason it's not impacting us atm is because we're over-rendering).
There are a lot of cases where the compiler skips a component that's valid. For example it doesn't fully support TypeScript "satisfies" syntax, here it bails, while here it works.
These type of issues aren't flagged without the __unstable_donotuse_reportAllBailouts option to avoid leading people to refactor code in ways to only please the current version of the compiler, instead of what's best for code readability and for the humans working in the project.

In this case we don't really care or show the bailed out components that aren't actionable, we just gobble it up in a JSON file to setup mutually exclusive source filters between Million Lint and React Compiler.
That way we get intel on which of the components that don't have react errors, but for whatever reason isn't compiled, is worth looking into manually memoizing in the mean time.

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.

1 participant