-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Conversation
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.
I haven't reviewed in detail but from a glance looks good. I see a lot of copying — wondering if you have any memory usage concern, and whether it's likely the integration will introduce more GC pressure to potentially skew the measurements. I'd like to play with it in the coming days (maybe let's set up a video call)?
agent/inject.js
Outdated
hook.sub('update', ({renderer, internalInstance, data}) => agent.onUpdated(internalInstance, data)), | ||
hook.sub('updateProfileTimes', ({renderer, internalInstance, data}) => agent.onUpdatedProfileTimes(internalInstance, data)), | ||
hook.sub('unmount', ({renderer, internalInstance}) => agent.onUnmounted(internalInstance)), |
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.
Is it worth keeping "basic" events necessary for the tree separate from "profiler" events?
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.
Not sure what you're suggesting.
The added "updateProfileTimes" event was necessary to handle the case where a component bailed out– something DevTools traditionally didn't care about, but now needs to handle so that we can update the actualDuration
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.
Sorry, I proposed to separate
// Basic functionality:
hook.sub('mount', ({renderer, internalInstance, data}) => agent.onMounted(renderer, internalInstance, data)), hook.sub('mount', ({renderer, internalInstance, data}) => agent.onMounted(renderer, internalInstance, data)),
hook.sub('update', ({renderer, internalInstance, data}) => agent.onUpdated(internalInstance, data)), hook.sub('update', ({renderer, internalInstance, data}) => agent.onUpdated(internalInstance, data)),
hook.sub('unmount', ({renderer, internalInstance}) => agent.onUnmounted(internalInstance)), hook.sub('unmount', ({renderer, internalInstance}) => agent.onUnmounted(internalInstance)),
// Necessary for profiler:
hook.sub('root', ({renderer, internalInstance}) => agent.addRoot(renderer, internalInstance)),
hook.sub('rootCommitted', ({renderer, internalInstance}) => agent.rootCommitted(renderer, internalInstance)),
hook.sub('updateProfileTimes', ({renderer, internalInstance, data}) => agent.onUpdatedProfileTimes(internalInstance, data)),
So it's clearer the new set of events are additional and aren't necessary for the main part to work.
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.
Ah! Yeah that makes sense 😄 I misunderstood.
</p> | ||
)} | ||
<p> | ||
<a href="https://reactjs.org/docs/interaction-tracking.html" target="_blank"> |
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.
Can we make this an fb.me URL and then add a todo list to point it to the right thing together with the release that includes it?
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.
Sure. Done.
Although I think fb.me URLs don't support being edited after they're created– so I'm not sure how much value this actually provides vs a direct reactjs.org link.
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.
They do support being edited. (It's behind a scary confirmation dialog but it works.)
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.
I thought they used to! But the last two times I've looked for a way to edit one, it doesn't appear to exist anymore, and the tool specifically says that you shouldn't edit them because they might be cached externally. Maybe this changed recently?
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.
Or maybe I just overlooked the dialog 😄 Hopefully that's all!
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.
It's weird because there are two tools.
Go to the one that says "avoid" and then add /create/
to its URL. You'll find the one that allows it (after clicking okay in a confirmation dialog).
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.
Oh nice! Thanks
It's possible, although I think the impact of this is minimal (compared to the immutable data structure we're already maintaining) unless you actually profile in which case... I still think we're maybe doing the best thing I could think of.
Sounds good to me! |
Okay– I'm landing this beast. I'll rebase my other less-clutter PR once it's landed and fix up the two issues you mentioned on it. Let's setup a time (at your convenience) to do a video chat about the profiler and if anything's uncovered, I'll do a follow up. Thanks! |
@bvaughn, is this feature actually supposed to work as of devTools v3.3.1? I see the Profiler tab, and I'm able to begin recording (or at least it looks that way according to the UI), but when I finish recording, I get nothing -- it looks as if no data has been collected. |
I thought we didn't show Profiler tab with 16.4.2.. 🤔 It wouldn't work. The code for it will be in the next patch. |
No, it isn't expected to work until we release the next minor. Sorry for the confusion.
The profiler shows if it detects a profiling-capable version of React, which it does by checking for the presence of a specific profiling attribute that actually does exist in 16.4. In hindsight, I should have checked for Regardless, the plan (based on feedback from Sophie) is to show the tab always and promote upgrading to a profiling-capable build of React if it's not supported. I was planning on making that change soon. As part of that, I'll change the feature detection to use |
Btw @jrnail23, I've just published 3.3.2 to the Chrome and Firefox stores that should fix this issue. Thanks! |
@bvaughn, thanks for the really quick turnaround on this! Any idea when I might expect to see v3.3.2 in the stores? |
No. It's out of my hands. Whenever Chrome team approves it. 😄 Usually within an hour or so. |
@bvaughn, to be clear, should I expect the profiling to work in 3.3.2, or did you just stop the tab from showing for now? |
I fixed the false positive feature detection. Profiling won't work until we release the next react version or if you're using a pre-built release with it enabled. |
Got it, @bvaughn, thanks again for being so responsive! |
@bvaughn Awesome work! I tried to install the pre-build release of devtool but the Profiler tab was not showing. Did I miss something obvious in here? Or currently, the Profiler tab was purposely disabled? |
The prerelease page mentions this (but maybe not boldly enough) – the profiler will only work (show up) if you're running the latest React (from master) or one of the pre-built packages also on the page. |
Sweet thanks! |
Builds on top of the experimental Profiler API and the "interactions" POC (facebook/react/pull/13253).
Blocking Dependencies
Profiler
duration bugfix: Profiler actualDuration bugfix react#13313Non-blocking Dependencies
The profiler will degrade gracefully if the interaction-tracking features are not available at runtime.
interaction-tracking
package: interaction-tracking package react#13234Profiler
andinteraction-tracking
integration: Profiler integration with interaction-tracking package react#13253Follow Up Work
Demo
Video walk throughs of profiler UI can be found here and here. Below are some screenshots.
No profiling data
Recording
No timing data for commit
Flame chart
Ranked chart
Interactions
No Interactions
Fiber render chart