React Rally profiler feedback umbrella issue

[bvaughn]

I did several live demos of the new DevTools profiler with people at React Rally. Some of them used pre-built demo apps and others profiled local builds of their own apps. Over all the feedback was very positive, but I gathered several suggestions and observations from the tests. It's likely that we may not be able to implement all of them, but they are:

Flamegraph

• 49b9e47, 5cf07b2: Flamegraph/ranked charts should enable you to deselect the current fiber so the right hand pane shows commit statistics again. Perhaps this should be a new button in the top of the panel. Clicking in an "empty" space of the flame graph should also deselect.
• e03c7da: Show un-rounded durations on hover in tooltips for more info
• Confirm that rounding isn't lopping off significant digits (all times ended up .00 in one of the demos)

Interactions

• 35c1f2a: Hide the snapshot selector in this view since it's not interactive and it may cause confusion.
• 09edccb: Tie the commits more to the chart views. Maybe use the colors (based on commit duration) for the circles in the interactions view?
• 09edccb: Commits are shown as bars in the charts, but in the interactions tab they're circles. Maybe they should be little colored squares? At least in the "renders" side pane, rather than showing the triangle that fills in, show the colored squares?
• 09edccb: Maybe use black for selected (like we do in the snapshot selector)

Horizontal snapshot selector

• d5047c9, ea8e92c: Make the selected commit more prominent in the horizontal selector than just an alpha transform. Too many people didn't notice it. Maybe add an overlay pattern so it stands out more?
• ad38466: Left/right arrow keys should step through commits/snapshots.
• a2907c8: Make sure jumping from fiber-durations bar chart view to flame/ranked also horizontally scrolls the snapshot selector.
• 0c79424: When dragging, don't leave the dragging state until you release the mouse. (Even if you mouse outside of the selector.)
• Scrollbar gets in the way– makes it hard to see the commits.

Right hand details pane

• 5cf07b2: Make the right hand details pane buttons blue so they stand out more as buttons.
• 6f075a5: Display "committed at" rather than "commit time".
• Profiler: Improve commit "diff" UX #1114: When a fiber+commit is selected, the right details pane could highlight props/state that are different from the previous commit. (This info flashes yellow when you step through commits, but it would be nice to highlight more permanently in this mode.)
• Profiler: Improve "render count" UX #1115: The render count label is confusing since it's a count of renders for the entire app lifetime and not just the window of time when we were recording. (Showing these counts only for when we are profiling might add a lot of weight to the profiler.)
• Profiler: Can't always expand props in right panel #1116: It's confusing that you can't always expand props (or can only expand them a few levels). This might be a bug, or maybe just bad UI.
• It's unclear if "render duration" means duration of the commit, or duration of reconciliation, or both. If it's not commit duration, can/should we add commit duration too?
• Can we add the context menu "Store as global variable" option to props here (like in the Elements tab)? Will this work for mutable data that's old and changed already?

Component durations bar chart

• 5e7de4e: It's not obvious which commits are visible in the component durations bar chart. Perhaps we could dim the commits thate are not visible in the above selector?
• The component durations bar chart was confusing to people at first.

Elements tree view

• e881623: Single-click on the tree view toggle arrow doesn't expand/collapse the tree anymore. (You have to double-click it.)
• b89dbb1: Nodes weren't wrapping correctly in Firefox when the extension was in a detached window. (This also pushed the right props panel off the screen.)
• Consider showing an indicator of which components in the tree are stateful

General

• 27e1f29: "Show native nodes" probably isn't that useful– maybe add a settings/gear icon there and have a profiling preferences popup?
• 802932b, e8c4ecb: Add a preference (in the new profiler preferences popup) to filter commits below a certain duration too (to reduce noise in commit selector).
• Highlight the owner tree when a node is focused #1094: Refactor the breadcrumb UI to remove host components. Preferably only show the owner stack.
• Profiler: Always show profiler tab (even when profiling not supported) #1117: Show Profiler tab always (not just if a profiling capable build of React is present). This tab can be used to up-sell a React upgrade if the version is too old to support profiling.
• Profiler: Add option to go to source (like elements context menu) #1119: Add a way to jump to source from a selected component (similar to the Element tab context menu).
• Profiler: Add import and/or export options #1120: Add an "export" option that dumps profiling data to some JSON format so that it can be shared.
• Profiler: Add import and/or export options #1120: Add an "import" option that can read in exported profiling data and display it. (Perhaps this could be done with a standalone version of the profiler, similar to the DevTools theme browser.)
• Profiler: Support "reload and start profiling" option #1121: Either resume recording if the DevTools (or a web page) are re-loaded while recording is active, or add a restart-and-profile button like Chrome "Performance" tab has. (This may not be possible due to initialization/timing issues.)

[gaearon]

Maybe add a preference (in the new profiler preferences popup) to filter commits below a certain duration too (to reduce noise in commit selector).

That was my concern from watching your videos too. 👍

[bvaughn]

It's also unclear if "render duration" means duration of the commit, or duration of reconciliation, or both. If it's not commit duration, can/should we add commit duration too?

I think I missed a bit of nuance when we were chatting about this yesterday, since I was using my mobile phone and I was out and a bit distracted.

This duration is the time spent in render phase, including lifecycles. We don't actually measure the commit phase. We really only call now() once in the commit phase (in commitRoot) to capture the commit time.

[dan-kez]

Hey Brian, thanks for showing me the profiler at React Rally - it's awesome!

After the suspend API presentations it occurred to me that we don't, from what I could tell, have a central place to see what data has been cached / what we've attempted to "fetch" yet.

It would be nice if there was a way to understand if any particular Resource has been fetched, or is presently fetching.

const UserName = ({ userId }) => (
  const user = UsersResource.read(cache, userId)
  return (<span>{user.name}</span>);
);

In the tools it would be nice to see distinct events for when UsersResource is initially called, when it returned, and subsequent reads from cache based on the arguments to read. It would be especially useful if there was a top level view similar to redux, rather than needing to hunt for the components where these calls are made.

That said, this definitely may be scope creep for the profiler.

[bvaughn]

Hi @dan-kez 👋Thank you, and you're welcome.

I'd suggest filing a new GitHub issue to discuss this ^

Currently, it would not be possible for DevTools to do what you're describing because it doesn't actually have access to the resource/cache. Since we own the simple-cache-provider package, it would be possible to build hooks into that package to enable this sort of functionality– but I'm not sure we would want to do that since it would add some overhead to the package. Seems like it merits a separate discussion to consider pros and cons 😄

[dan-kez]

Sounds great - thanks for the quick response!

I just opened an issue to discuss exposing these hooks while in __DEV__ mode in the react repo.

[gaearon]

We don't actually measure the commit phase. We really only call now() once in the commit phase (in commitRoot) to capture the commit time.

Interesting. My impression was that especially with async rendering, the commit duration will become more important because it can’t be time sliced. Long commits are bad. But overall render duration is less important if it was deferred and time sliced successfully.

[gaearon]

See also facebook/react#9075

[bvaughn]

the commit duration will become more important because it can’t be time sliced

Yeah, this is my understanding too. Commit phase is very time sensitive– but because of this, we also wanted to avoid starting/stopping a bunch of timers and adding/bubbling a bunch of durations.

[jesup]

Concerning import/export of profiles.... I haven't looked at the in-devtools version, but the perf-html.io version has JSON export and imports JSON (and soon linux perf and native mac profiles)

[swyxio]

mini-issue: 1
minor UX issue re: Horizontal snapshot selector - currently the left/right arrow keys wrap around when at either end of the snapshots. (i.e. when i'm at the 1st snapshot and press left, it wraps to the last snapshot). i believe it should not wrap.

[swyxio]

mini-issue: 2
another: "Store as global variable" works if you right click on a variable in Elements: https://swyx.tinytake.com/sf/Mjg2MDA1MV84NTg0OTc0
however in Profiler this does not work: https://swyx.tinytake.com/sf/Mjg2MDA1NV84NTg0OTkw

[bvaughn]

Thanks for the feedback @sw-yx 😄

minor UX issue re: Horizontal snapshot selector - currently the left/right arrow keys wrap around when at either end of the snapshots. (i.e. when i'm at the 1st snapshot and press left, it wraps to the last snapshot). i believe it should not wrap.

I already made this change after doing a user test with Shirley (see 0e983a1). Try the publicly released version of the DevTools rather than my pre-release build. It has a few new goodies. 😄

[swyxio]

maybe not so mini-issue: 3

so generally there's "vertical" and "horizontal" perf issues - "vertical" is when an individual frame has an expensive render, "horizontal" is when too many renders are generated by nonperformant code (i call it horizontal because too many frames). this profiler is optimized for the "vertical", but arguably "horizontal" perf issues (like mine)are at least as common if not more common.

I OOM'ed a few times while using the devtools to do horizontal perf profiling. i know you collect a lot of data for profiling and this may be too much to ask, but i wonder if a "lite record" option is possible that basically only identifies the render counts of each component. that would help a lot already and would be more robust to really crappy code like I probably write.

[bvaughn]

i wonder if a "lite record" option is possible that basically only identifies the render counts of each component. that would help a lot already and would be more robust to really crappy code like I probably write.

Most things are possible, but this would depend on what causes the OOM. DevTools uses Immutable and already accumulates a good bit of data through regular usage. The profiler shouldn't be adding too much on top of that– since most of the profiler data is lazily calculated only when you view the charts. But we do cache it from there out, so it's faster to step between snapshots.

If you'd be interested in lending a hand here, since you're able to repro this case, I'd suggest turning that caching off (e.g. just make these methods) no-ops) and see if that (a) fixes your OOM and (b) doesn't negatively impact performance too much. If so, I'm happy to turn it off. 😄

Edit I'm going to try converting my cache-everything approach to an LRU cache. Maybe that will help both issues. Stay tuned...

[bvaughn]

Hey proponents of stateful labels in the tree view– is this too subtle to be useful?

I don't think I like it. I tried a badge (similar to Firefox's "events" badge) but the vertical alignment doesn't match up properly with the ">" and I couldn't stand it. (Also the padding takes up even more space.)

I'm trying to give this idea a fair shake though.

diff --git a/frontend/Node.js b/frontend/Node.js
index f0eef23..e2c4ebe 100644
--- a/frontend/Node.js
+++ b/frontend/Node.js
@@ -286,6 +286,13 @@ class Node extends React.Component<PropsType, StateType> {
       color: isWindowFocused ? getInvertedWeak(theme.state02) : 'inherit',
     };
 
+    let stateTag = null;
+    if (nodeType === 'Composite' && node.get('state') != null) {
+      stateTag = (
+        <small>&nbsp;state</small>
+      );
+    }
+
     // Single-line tag (collapsed / simple content / no content)
     if (!children || typeof children === 'string' || !children.length) {
       const jsxSingleLineTagStyle = jsxTagStyle(inverted, nodeType, theme);
@@ -313,6 +320,7 @@ class Node extends React.Component<PropsType, StateType> {
                 &gt;
               </span>,
             ]}
+            {isCollapsed ? null : stateTag}
             {selected && <span style={dollarRStyle}>&nbsp;== $r</span>}
           </div>
         </div>
@@ -325,6 +333,7 @@ class Node extends React.Component<PropsType, StateType> {
         &lt;/
         <span style={jsxCloseTagStyle}>{name}</span>
         &gt;
+        {collapsed && stateTag}
         {selected && ((collapsed && !this.props.isBottomTagSelected) || this.props.isBottomTagSelected) &&
           <span style={dollarRStyle}>&nbsp;== $r</span>
         }
@@ -355,6 +364,7 @@ class Node extends React.Component<PropsType, StateType> {
           <Props key="props" props={node.get('props')} inverted={headInverted}/>
         }
         &gt;
+        {!collapsed && stateTag}
         {selected && !collapsed && !this.props.isBottomTagSelected &&
           <span style={dollarRStyle}>&nbsp;== $r</span>
         }

[gaearon]

I don't like it. We also had a different style for triangles for stateful components and nobody noticed when we removed it (because nobody used it).

I think the reason people want to see state is because they're tracing the owner tree. Which is why we should help them with e.g. breadcrumbs.

[bvaughn]

Cool. I was on the fence anyway. I'll mark as won't-do for now 😄 Breadcrumb approach sounds better.

[bvaughn]

@sw-yx Try the latest build from react-devtools-profiler-prerelease.now.sh. I replaced the simple caching strategy with an LRU one to limit the memory impact on larger profiles. Does it improve things? If not, I'd like more data on how to repro.

[bvaughn]

Think I may have gotten most of the low hanging fruit for this PR. I might go through the remaining work and file new issues (or just close in a few cases as "won't fix" for now).

[swyxio]

sorry just saw this @bvaughn - trying now, will edit this comment either way. fingers crossed

EDIT: yup, this is great!!! i used to OOM with ~600 commits, and now i just went up to 2000 no problem. thanks so much for doing whatever you did! guess those interview algorithms are useful after all?

EDIT: I don't consider this at all necessary to handle but just reporting that in my test with 10,000 commits I got this error message 16 times:

Uncaught Error: Message length exceeded maximum allowed length.
    at PortImpl.postMessage (VM1295 extensions::messaging:121)
    at Port.publicClassPrototype.(:8080/sites/amazing-noether-dc5336/anonymous function) [as postMessage] (extensions::utils:138:26)
    at handleMessageFromPage (contentScript.js:24)

maybe worth throwing in a try/catch, maybe not. wouldn't spend too much time on it if it takes more work than adding a try/catch :)

[bvaughn]

Thanks for the update, Shawn! Very happy to hear that.

Thanks for the note about the message length issue. Not sure if that's easy to fix. Will see.