-
Notifications
You must be signed in to change notification settings - Fork 47k
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
refactor[react-devtools/fiber/renderer]: optimize durations resolution #31118
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
976c607
to
830e823
Compare
duration: | ||
formatDurationToMicrosecondsGranularity(maxActualDuration), | ||
effectDuration: | ||
effectDuration !== null | ||
? formatDurationToMicrosecondsGranularity(effectDuration) | ||
: null, | ||
fiberActualDurations, | ||
fiberSelfDurations, | ||
passiveEffectDuration, | ||
passiveEffectDuration: | ||
passiveEffectDuration !== null | ||
? formatDurationToMicrosecondsGranularity(passiveEffectDuration) | ||
: null, |
There was a problem hiding this comment.
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)
|
||
// 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; | ||
} |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
Also, do we have a rough estimate of how much bandwidth this would save/visible speed improvement? Very intrigued as I never thought of using number precision as an optimisation. Big if true! |
React DevTools profiling payload has data for each commit. Every commit will have 2 collections: [ In this PR we are mainly optimizing both It is safe to do it, since we don't actually use it from the Frontend side, see the description. |
Stacked on #31118. See last commit. We don't need to call `startProfiling()` here, because we delegate this to the Renderer itself: https://github.com/facebook/react/blob/830e823cd2c6ee675636d31320b10350e8ade9ae/packages/react-devtools-shared/src/backend/fiber/renderer.js#L5227-L5232 Since this is de-facto the constructor of Renderer, this will be called earlier. Validated via testing the reload-to-profile for Chrome browser extension.
Changes in this release: * Fix React Compiler badging ([poteto](https://github.com/poteto) in [#31196](#31196)) * fix[react-devtools]: fixed timeline profiler tests ([hoxyq](https://github.com/hoxyq) in [#31261](#31261)) * fix[react-devtools]: record timeline data only when supported ([hoxyq](https://github.com/hoxyq) in [#31154](#31154)) * refactor[react-devtools]: flatten reload and profile config ([hoxyq](https://github.com/hoxyq) in [#31132](#31132)) * fix[react-devtools]: remove all listeners when Agent is shutdown ([hoxyq](https://github.com/hoxyq) in [#31151](#31151)) * fix[react-devtools]: removed redundant startProfiling call ([hoxyq](https://github.com/hoxyq) in [#31131](#31131)) * refactor[react-devtools/fiber/renderer]: optimize durations resolution ([hoxyq](https://github.com/hoxyq) in [#31118](#31118)) * fix[react-devtools]: update profiling status before receiving response from backend ([hoxyq](https://github.com/hoxyq) in [#31117](#31117)) * fix[react-devtools]: wrap key string in preformatted text html element ([hoxyq](https://github.com/hoxyq) in [#31153](#31153)) * chore[react-devtools]: drop legacy context tests ([hoxyq](https://github.com/hoxyq) in [#31059](#31059)) * chore[react-devtools]: add legacy mode error message to the ignore list for tests ([hoxyq](https://github.com/hoxyq) in [#31060](#31060)) * fix[react-devtools]: request hook initialization inside http server response ([hoxyq](https://github.com/hoxyq) in [#31102](#31102)) * [Flight] Serialize Server Components Props in DEV ([sebmarkbage](https://github.com/sebmarkbage) in [#31105](#31105)) * Add: reload to profile for Fusebox ([EdmondChuiHW](https://github.com/EdmondChuiHW) in [#31021](#31021)) * refactor: allow custom impl of backend realod-to-profile support check ([EdmondChuiHW](https://github.com/EdmondChuiHW) in [#31048](#31048)) * fix: use public instance in Fiber renderer and expose it from getInspectorDataForViewAtPoint ([hoxyq](https://github.com/hoxyq) in [#31068](#31068))
Stacked on #31117.
No need for sending long float numbers and to have resolution less than a microsecond, we end up formatting it on a Frontend side:
react/packages/react-devtools-shared/src/devtools/views/Profiler/utils.js
Lines 359 to 360 in 6c7b41d