Profiler integration with interaction-tracking package #13253
Conversation
|
ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1% Details of bundled changes.Comparing: 2967ebd...e61eb6c react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
bvaughn
changed the title
| return; | ||
| } | ||
| if (__DEV__) { | ||
| invariant( |
There was a problem hiding this comment.
Ah, is our current convention just to throw explicitly?
There was a problem hiding this comment.
Well maybe if this is going to be a separate package. But what I meant is we shouldn't have dev- or prod-only invariants.
|
|
||
| let root = finishedWork; | ||
| while (root.return !== null) { | ||
| root = root.return; |
There was a problem hiding this comment.
Let's pass this to commitWork instead
| onRender( | ||
| finishedWork.memoizedProps.id, | ||
| current === null ? 'mount' : 'update', | ||
| finishedWork.actualDuration, | ||
| finishedWork.treeBaseDuration, | ||
| finishedWork.actualStartTime, | ||
| getCommitTime(), | ||
| interactions === null ? [] : Array.from(interactions), |
| // Secondly, this is how commitWork() will access them to pass them to any Profiler onRender() hooks. | ||
| // commitWork() can't use getInteractionsForExpirationTime() because the interactions have already been cleared. | ||
| // We can't wait to clear them out of the Map after commitWork() either, or we'll wipe out cascading updates. | ||
| finishedWork.stateNode.committedInteractions = interactions; |
There was a problem hiding this comment.
You can move this and the other assignment in createFiberRoot to the begin phase. Also let's call it memoizedInteractions instead? "Committed" is misleading because it hasn't committed yet at this point. It's more like memoizedProps and memoizedState.
| @@ -1043,6 +1070,19 @@ function renderRoot( | |||
|
|
|||
| const expirationTime = root.nextExpirationTimeToWorkOn; | |||
|
|
|||
| let interactions = null; | |||
There was a problem hiding this comment.
Nit: This can go inside the if
bvaughn
changed the title
|
Chatted with Sebastian about how heavy-weight the implementation from the weekend has become. Here's some notes for myself about a possible alternative API that we'll need to discuss with web speed folks:
|
bvaughn
changed the title
|
Sebastian, Andrew, and I continued talking yesterday at some length and came up with a new proposed observer API documented here. I've just pushed this implementation, and tests, and will take a pass at updating the PR description now. |
| if (enableInteractionTracking) { | ||
| ((root: any): FiberRoot).interactionThreadID = getThreadID(); | ||
| ((root: any): FiberRoot).memoizedInteractions = new Set(); | ||
| ((root: any): FiberRoot).pendingInteractionMap = new Map(); |
There was a problem hiding this comment.
Can adding these later affect hidden class? I wonder if that or the number of fields itself could mess with some optimizations. Seems like it's important that we try to make the profiling build as close to prod characteristics as possible.
There was a problem hiding this comment.
Hmm! I don't know. If so, I could move the definition of root within the if/else to ensure that we always added the same number of fields based on the feature flag.
| const threadID = | ||
| committedExpirationTime * 1000 + root.interactionThreadID; | ||
| ((__subscribers: any): Subscribers).forEach(observer => | ||
| observer.onWorkStopped( |
There was a problem hiding this comment.
Related to my concerns in #13234 (comment), I think we need to be very confident in user-level code to call it from this place in the scheduler. Since it's not inside the render phase or one of protected places in the commit phase, there's a risk that any error in it (and folks who want to use it are humans and may make mistakes) can mess up the scheduler state and cause infinite loops etc.
Or we could wrap any user code call like this in a try/catch.
There was a problem hiding this comment.
Great point. Although I'm not sure what the expected error recover behavior really should be in this case. I'll take a stab at it– with some new tests– and we can discuss it from there.
| // Wrap suspense callback so that the current interaction are preserved. | ||
| // Interaction threads are unique per root and expiration time. | ||
| const threadID = | ||
| renderExpirationTime * 1000 + root.interactionThreadID; |
There was a problem hiding this comment.
Will it break if I change the formula here but not in the other place it's used?
There was a problem hiding this comment.
It will "break" in that the callbacks will receive a different thread ID and so any reporting done based on those callbacks would be inaccurate. I'll add a comment to each of these places noting that we shouldn't change one without changing the others. Good call!
bvaughn
changed the title
| finishedWork.treeBaseDuration, | ||
| finishedWork.actualStartTime, | ||
| getCommitTime(), | ||
| Array.from((root.memoizedInteractions: any)), |
There was a problem hiding this comment.
Why pay the cost of cloning this data? Why not just pass the set and let the consumer decide?
There was a problem hiding this comment.
memoizedInteractions is a Set and the I initially proposed passing onRender an Array. We could change this so that the API passes a Set instead, then I wouldn't need to clone it.
Users could mutate it though, and I think a Set can't be object-frozen...?
Open to suggestions here 😄
| interactionThreadID: getThreadID(), | ||
| memoizedInteractions: new Set(), | ||
| pendingInteractionMap: new Map(), | ||
| }; |
There was a problem hiding this comment.
Good VM field hints. Nice.
| @@ -338,7 +346,7 @@ function resetStack() { | |||
| nextUnitOfWork = null; | |||
| } | |||
|
|
|||
| function commitAllHostEffects() { | |||
| function commitAllHostEffects(root: FiberRoot) { | |||
There was a problem hiding this comment.
Hm. Unfortunate to pass another arg just for this. @acdlite does this look right to you? Can we get this from somewhere else?
There was a problem hiding this comment.
Should be able to use nextRoot, but I assume Brian has passed it as an argument to avoid another type check, since Flow doesn't know that it's not null.
There was a problem hiding this comment.
The chain is ... -> performWorkOnRoot(root) -> completeRoot(root) -> commitRoot(root) -> commitAllHostEffects(root)
Why is passing root to this specific function a problem? 😄
There was a problem hiding this comment.
Some functions get inlined so those variables don't matter much.
Extra variables can be much worse than closures or property reads. Because if we tip over the number of variables used then the VM will try to guess which ones to pop out of the register and back to main memory.
The key here is that this variable isn't used by the mainline path here. Just the special case path of the profiler.
There was a problem hiding this comment.
Should be able to use
nextRoot, but I assume Brian has passed it as an argument to avoid another type check, since Flow doesn't know that it's not null.
Maybe I'm missing something... nextRoot is actually null in many cases though.
renderRoot() sets nextRoot to null to indicate that there's no more work to be done for the current batch. This means that if performWorkOnRoot() then calls completeRoot() – the value will be null.
I assume this is why we've been passing around a root param to these other functions in the first place.
| // correct value. | ||
| didTimeout: false, | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
Use try/finally instead. This is what it is for.
There was a problem hiding this comment.
I believe catch is appropriate here, because I specifically want to reset the scheduledCallback only when an error occurs that prevents it from being processed.
| // correct value. | ||
| didTimeout: false, | ||
| }); | ||
| } catch (error) { |
There was a problem hiding this comment.
Same as above. (I think catch is right here.)
| let prevInteractions: Set<Interaction> | null = null; | ||
| let committedInteractions: Array< | ||
| Interaction, | ||
| > | null = enableInteractionTrackingObserver ? [] : null; |
There was a problem hiding this comment.
Unconditionally allocating an array for every commit seems unfortunate.
| if (enableInteractionTracking) { | ||
| // We're about to start new tracked work. | ||
| // Restore pending interactions so cascading work triggered during the render phase will be accounted for. | ||
| prevInteractions = ((__interactionsRef: any): InteractionsRef).current; |
There was a problem hiding this comment.
All these any casts are too fragile. Let’s make sure that we cast this at the export type in one place.
| // DO NOT CHANGE THIS VALUE without also updating other threadID values. | ||
| const threadID = | ||
| renderExpirationTime * 1000 + root.interactionThreadID; | ||
| onResolveOrReject = wrap(onResolveOrReject, threadID); |
There was a problem hiding this comment.
This is not obvious to me. Wouldn’t there already be an interaction tracked for this expiration time? And these callbacks can’t do arbitrary things. They just schedule work on something that is already scheduled. So seems to me that this shouldn’t need to be wrapped.
There was a problem hiding this comment.
Yes, we're within an active interaction zone at this point. (That's why wrap works in the first place.)
The issue is that we attach onResolveOrReject to our thenable to resume work when the e.g. promise has finished loading. wrap ensures that we restore the right set of interactions at that time.
Without this, our interaction zone expires when React finishes this batch of work– and there's nothing to restore it when the thenable completes.
There was a problem hiding this comment.
Without this, our interaction zone expires when React finishes this batch of work– and there's nothing to restore it when the thenable completes.
This is only an issue if the work times out, which we determine here: https://github.com/facebook/react/blob/master/packages/react-reconciler/src/ReactFiberScheduler.js#L1449
There was a problem hiding this comment.
Sorry, maybe I'm being dense, but I'm not sure I understand how this feedback applies.
| @@ -1988,6 +2166,40 @@ function performWorkOnRoot( | |||
| 'by a bug in React. Please file an issue.', | |||
| ); | |||
|
|
|||
| if (enableInteractionTracking) { | |||
There was a problem hiding this comment.
While everything below here makes sense. This is it the right place for it.
This gets called for every scheduled work (requestIdleCallback).
We only want to call this when we’re restarting work on a root. Not when we’re resuming.
@acdlite Can probably point to the right place to do this.
There was a problem hiding this comment.
There was a problem hiding this comment.
Hm, I think this breaks the suspense case– (e.g. covered by test "tracks both the temporary placeholder and the finished render for an interaction")– since renderRoot only resets the stack once but commitRoot is potentially called multiple times. (This causes the interaction pending working "count" to be off.)
| ); | ||
|
|
||
| if (caughtError) { | ||
| throw caughtError; |
There was a problem hiding this comment.
try/finally will just rethrow the error by default. You can remove the catch and this line.
There was a problem hiding this comment.
Right. This one's a bit tricky, but I'm manually catching and re-throwing to guard against an error in one of the onInteractionScheduledWorkCompleted callbacks– so that it doesn't block subsequent callbacks. (I'm iterating over a set of interactions and calling this callback for any that have completed.)
|
I'm about to force-push a cleaned up version of this branch. All of the original commits have been squashed and the branch has be rebased on master. The PR responses made so far are in atomic commits so they should be easy to spot. I haven't made some of them (e.g. try/finally) because I think they're misunderstandings. I mention the force push because it might clobber some of the comments Sebastian and I have left so far. (Sorry.) |
bvaughn
force-pushed
the
interaction-tracking-profiler-integration
branch
from
f7693b3 to
956674a
Compare
…hrow more safely. Reverted try/finally change to ReactTestRendererScheduling
bvaughn
force-pushed
the
interaction-tracking-profiler-integration
branch
from
676e3ca to
c9b6c87
Compare
|
Andrew, Sebastian, and I chatted about adding a new interaction-tracking NPM dependency to renderers (e.g. react-dom, react-native, etc) and I think we're all okay with this. The only downside is that Rollup DCE isn't stripping the unused |
|
We should most definitely avoid this in production bundle. This should be a blocker for cutting a release. If you need help I’d be happy to look — just let me know. |
* Remove interaction-tracking wrap() references from unwind work in favor of managing suspense/interaction continuations in the scheduler * Moved the logic for calling work-started hook from performWorkOnRoot() to renderRoot()
Hey @gaearon 👋 Great! Let's chat. It's possible we're thinking about slightly different things– but if not I'd love to get your input. 😄 FWIW, I was planning to add a post-build step that strips the unused import out of the production bundle– but as a follow up PR. |
| @@ -1929,9 +1925,9 @@ function onTimeout(root, finishedWork, suspendedExpirationTime) { | |||
| if (enableInteractionTracking) { | |||
| // Don't update pending interaction counts for suspense timeouts, | |||
| // Because we know we still need to do more work in this case. | |||
| freezeInteractionCount = true; | |||
| suspenseDTimeout = true; | |||
There was a problem hiding this comment.
Nah I just can't type for shit
bvaughn
force-pushed
the
interaction-tracking-profiler-integration
branch
from
b308fee to
a173f36
Compare
| @@ -217,7 +217,7 @@ function throwException( | |||
| : renderExpirationTime; | |||
|
|
|||
| // Attach a listener to the promise to "ping" the root and retry. | |||
| const onResolveOrReject = retrySuspendedRoot.bind( | |||
| let onResolveOrReject = retrySuspendedRoot.bind( | |||
There was a problem hiding this comment.
Nit: change back to const plz
|
Seems fine to fix the packaging issue in a follow up (before we release) |
|
Thanks for the thorough review and feedback folks! 🙇 |
| nextScheduledRoot: null, | ||
| }: BaseFiberRootProperties); | ||
| } | ||
|
|
There was a problem hiding this comment.
@bvaughn Can I ask that why we do the repetitive logic, i.e. only interactionThreadID, memoizedInteractions and pendingInteractionMap is related to enableInteractionTracking, we don't need to copy the remaining fields here, why not just do something like if (enableInteractionTracking) { root.interactionThreadID === xxx} or if (enableInteractionTracking) { root === { ...root, interactionThreadID: xxx } }( or use Object.assign here).
There was a problem hiding this comment.
We do it for performance since this code is really "hot" (it runs a lot). Seb would be a much better person to answer this question but I'll take a stab at it. 😄
JavaScript VM makes property lookup faster for objects by creating a class in the native-code layer based on the "shape" of the object (e.g. what keys it has). This is called the "hidden class", and it's a runtime optimization. If the VM isn't sure about the object's shape, it uses a slower (hash table) method to lookup properties. When fields are added/deleted after an object is created, it breaks the VM's "hidden class" assumptions and de-opts to hash table lookup.
Probably worth remembering that enableInteractionTracking is a compile time flag so this if/else won't exist in the built bundle. 😄
There was a problem hiding this comment.
http://v8-io12.appspot.com/#29
https://www.youtube.com/watch?v=UJPdhx5zTaw
Thank you Brian! I know the performance now.
* Updated suspense fixture to use new interaction-tracking API * Integrated Profiler API with interaction-tracking API (and added tests) * Pass interaction Set (rather than Array) to Profiler onRender callback * Removed some :any casts for enableInteractionTracking fields in FiberRoot type * Refactored threadID calculation into a helper method * Errors thrown by interaction tracking hooks use unhandledError to rethrow more safely. Reverted try/finally change to ReactTestRendererScheduling * Added a $FlowFixMe above the FiberRoot :any cast * Reduce overhead from calling work-started hook * Remove interaction-tracking wrap() references from unwind work in favor of managing suspense/interaction continuations in the scheduler * Moved the logic for calling work-started hook from performWorkOnRoot() to renderRoot() * Add interaction-tracking to bundle externals. Set feature flag to __PROFILE__ * Renamed the freezeInteractionCount flag and replaced one use-case with a method param * let -> const * Updated suspense fixture to handle recent API changes
TODO before merge
interaction-tracking/subscriberstarget via PR Add interaction-tracking/subscriptions #13426interaction-tracking/subscribersFix the build process so that theThis will be handle via a follow-up task. See this comment for more context.interaction-trackingimports/requires are stripped if theenableInteractionTrackingfeature flag is not enabled. Rollup is not currently removing the unused require.Implementation details
For the purposes of this PR, I have created a standalone package (interaction-tracking) that events can be registered with. Usage of this package is optional, and it has no dependencies on React. We may not end up using this implementation, but will probably use something that's conceptually similar.
Scheduling work for an interaction
Profiling builds of React track when renders are caused by an interaction(s). React does this by asking the interaction-tracking library if it's currently tracking anything when new work is scheduled. (This is done inside of
scheduleWorkandscheduleCallbackWithExpirationTime.) When there are active interactions, React stores them– along with the expiration time they're being scheduled with– on theFiberRootin apendingInteractionMapMap.Associating an interaction with async work
When React begins working on a new batch of work (in
performWorkOnRoot) it scans thependingInteractionMapMap and filters by the current expiration time in order to determine which interactions scheduled the current work. This filtered list of interactions is stored on the root asmemoizedInteractions.Before calling any user code (in
commitRootandrenderRoot) React temporarily restores these cached interactions using an advanced part of the interaction-tracking package API. This ensures that new async work (cascading work) that's created will also be associated with the original interaction(s).React also manges notifying any interaction subscribers when new work has been started/stopped.
Notifying
ProfilercallbacksWhen committing a
Profiler, React reads thememoizedInteractionsSet from the current root and passes the interactions along to theonRendercallback.DevTools integration
When the DevTools
onCommitRootis called, it reads thememoizedInteractionsSet from the committed root in order to determine if any interactions were processed.Open questions
Fiberto store the pending interactions when an update is scheduled. Then in begin/complete work, we'd have to start/stop (push/pop) continuations to properly handle cascading updates and to ensure the rightonRenderparams.