Skip to content

Record "actual" times for all Fibers within a Profiler tree #12891

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Record "actual" times for all Fibers within a Profiler tree #12891

wants to merge 6 commits into from

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented May 23, 2018 •
edited
Loading

Move "actual" time tracking attributes onto the Fiber object and track for every Fiber within a Profiler tree. This change is being made so that DevTools has the ability to display timing info for all components in an application (or a subtree) simply by flipping the root Fiber and its alternate to ProfileMode.

This change should have no impact on the production bundle since the newly added fields are stripped out if enableProfilerTimer is disabled. It should have minimal impact on the dev and production profiling bundles since measurements are only recorded if ProfileMode is active.

Checklist

  • All existing unit tests pass.
  • Bundle size isn't impacted for non-profiling builds. (See below.)
  • Reset "actual" time durations for all Fibers after commit- even ones that didn't update due to bailouts. (Otherwise their durations will accumulate for the next eventual re-render.)
  • Don't reset durations before DevTools commit hook is called. (Otherwise DevTools will be unable to read these values for profiling mode.)

Bundle sizes

Built after rebasing on master:
screen shot 2018-05-25 at 9 52 24 am

@bvaughn bvaughn requested review from gaearon, sebmarkbage and acdlite and removed request for gaearon and sebmarkbage May 23, 2018 18:13
@bvaughn
Copy link
Contributor Author

bvaughn commented May 24, 2018 •
edited
Loading

There's still an open question about how DevTools can read this info before it's reset.

Currently the onCommitRoot hook is called after the actualDuration values have already been reset. If we had an async, post-commit phase, we could reset the times there. (Or we could add a third field to flag them for reset in a subsequent render phase.)

Maybe we can change React to call the DevTools onCommitFiberRoot earlier in the commit phase (before these values have been reset)? I'd prefer to make this change in a follow up PR, in case it introduces any unexpected problems.

Edit Resolved by commit a34c8a8

@bvaughn bvaughn changed the title Record "actual" times for all Fibers within a Profiler tree [WIP] Record "actual" times for all Fibers within a Profiler tree May 25, 2018
@bvaughn
Copy link
Contributor Author

bvaughn commented May 25, 2018 •
edited
Loading

I've realized there is a pretty big flaw with the way I'm resetting (or more accurately not resetting) actual time durations on non-Profiler fibers. I'm going to mark this back as WIP while I think about it.

Essentially, fibers that don't get flagged with an update effect won't be processed during commit in a way that enables us to reset their timers, and I don't want to introduce something expensive to the "commit" phase (e.g. introducing a new commit effect type, traversing the tree) to fix this.

The best way of doing this that I have thought of is to use a (global) commit batch identifier that's updated once during the commit phase. Each (profiled) fiber has a batch ID as well, and when actual time is started- this ID is checked in order to determine whether to reset or accumulate duration. This has the downside of adding an additional field to fibers (for profiling builds only) but the upside of containing the writes to "render" phase and also addressing the DevTools limitations above.

Edit Proposed change added in commit a34c8a8

@bvaughn bvaughn changed the title [WIP] Record "actual" times for all Fibers within a Profiler tree Record "actual" times for all Fibers within a Profiler tree May 25, 2018
Brian Vaughn added 5 commits May 25, 2018 09:48
Moved actual time fields from Profiler stateNode to Fiber
2aa7da1
Record actual time for all Fibers within a ProfileMode tree
5005f08
Reset actualDuration for all Fibers (not just Profilers)
b66a44a
Add commit batch ID to Profiler
6473dd6 
A global 'batch id' is updated during the commit phase. Each (profiled) Fiber has a local batch id.

When the Profiler timer is started- the local id is compared to the global one in order to determine whether to reset or accumulate duration.

This has the downside of adding an additional field to fibers (for profiling builds only) but the upside of containing the writes to 'render' phase and allowing the DevTools commit hook to run before durations have been cleared out (so it can read them for profiling mode).
Reverted unnecessary change to ReactFiberCommitWork
3c03bb3
@pull-bot
Copy link

pull-bot commented May 25, 2018 •
edited
Loading

Details of bundled changes.

Comparing: 76e0707...05fa464

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% +0.1% 621.6 KB 622.3 KB 144.79 KB 144.96 KB UMD_DEV
react-dom.development.js +0.1% +0.1% 605.96 KB 606.67 KB 140.78 KB 140.95 KB NODE_DEV
ReactDOM-dev.js +0.1% +0.1% 628.26 KB 628.96 KB 143.54 KB 143.71 KB FB_WWW_DEV
ReactDOM-prod.js -0.0% 🔺+0.1% 273.74 KB 273.71 KB 51.53 KB 51.56 KB FB_WWW_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% +0.2% 403.87 KB 404.58 KB 90.23 KB 90.4 KB UMD_DEV
react-art.development.js +0.2% +0.2% 329.73 KB 330.43 KB 71.34 KB 71.5 KB NODE_DEV
ReactART-dev.js +0.2% +0.2% 335.25 KB 335.95 KB 70.72 KB 70.88 KB FB_WWW_DEV
ReactART-prod.js -0.0% 🔺+0.1% 146.84 KB 146.82 KB 25.35 KB 25.37 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.2% +0.2% 339.29 KB 340 KB 73.33 KB 73.51 KB UMD_DEV
react-test-renderer.development.js +0.2% +0.2% 330.12 KB 330.83 KB 70.59 KB 70.75 KB NODE_DEV
ReactTestRenderer-dev.js +0.2% +0.2% 335.96 KB 336.67 KB 70.17 KB 70.34 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.2% +0.2% 323.8 KB 324.51 KB 68.58 KB 68.75 KB NODE_DEV
react-reconciler-persistent.development.js +0.2% +0.2% 322.36 KB 323.07 KB 67.98 KB 68.14 KB NODE_DEV

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.2% +0.2% 457.44 KB 458.14 KB 100.02 KB 100.19 KB RN_FB_DEV
ReactNativeRenderer-prod.js -0.0% 🔺+0.1% 205.83 KB 205.81 KB 35.96 KB 35.99 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.2% +0.2% 457.1 KB 457.8 KB 99.95 KB 100.12 KB RN_OSS_DEV
ReactFabric-dev.js +0.2% +0.2% 448.35 KB 449.05 KB 97.77 KB 97.95 KB RN_FB_DEV
ReactFabric-dev.js +0.2% +0.2% 448.39 KB 449.09 KB 97.79 KB 97.96 KB RN_OSS_DEV

Generated by 🚫 dangerJS

@bvaughn bvaughn force-pushed the profile-mode-part-4 branch from 68501a2 to 3c03bb3 Compare May 25, 2018 16:53
@bvaughn bvaughn mentioned this pull request May 25, 2018
Merged
@bvaughn
Copy link
Contributor Author

bvaughn commented May 25, 2018

Closing in favor of PR #12910

@bvaughn bvaughn closed this May 25, 2018
@bvaughn bvaughn deleted the profile-mode-part-4 branch May 25, 2018 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaearon gaearon Awaiting requested review from gaearon

@acdlite acdlite Awaiting requested review from acdlite

@sebmarkbage sebmarkbage Awaiting requested review from sebmarkbage

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bvaughn @pull-bot @facebook-github-bot