Update user timing to record the timeout deadline with 'waiting' events #12479
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**what is the change?:** When we are processing work during reconciliation, we have a "timeout" deadline to finish the work. It's a safety measure that forces things to finish up synchronously if they are taking too long. The "timeout" is different depending on the type of interaction which triggered the reconciliation. We currently have a shorter "timeout" for "interactive updates", meaning we will try to finish work faster if the reconciliation was triggered by a click or other user interaction. For collecting more data in our logs we want to differentiate the 'waiting for async callback...' events based on the "timeout" so I'm adding that to the logging. One interesting note - in one of the snapshot tests the "timeout" was super high. Going to look into that. **why make this change?:** Right now we are debugging cases where an interaction triggers a reconciliation and the "waiting for async callback...' events are too long, getting blocked because the main thread is too busy. We are keeping logs of these user timing events and want to filter to focus on the reconciliation triggered by interaction. **test plan:** Manually tested and also updated snapshot tests. (Flarnie will insert a screenshot)
gaearon
reviewed
Mar 28, 2018
| @@ -218,7 +218,7 @@ exports[`ReactDebugFiberPerf measures deprioritized work 1`] = ` | |||
| ⚛ (Committing Host Effects: 1 Total) | |||
| ⚛ (Calling Lifecycle Methods: 0 Total) | |||
|
|
|||
| ⚛ (Waiting for async callback...) | |||
| ⚛ (Waiting for async callback... timeout is 10737418210) | |||
There was a problem hiding this comment.
This is probably because <div hidden> doesn't ever expire.
gaearon
reviewed
Mar 28, 2018
| @@ -1,7 +1,7 @@ | |||
| // Jest Snapshot v1, https://goo.gl/fbAQLP | |||
|
|
|||
| exports[`ReactDebugFiberPerf captures all lifecycles 1`] = ` | |||
| "⚛ (Waiting for async callback...) | |||
| "⚛ (Waiting for async callback... timeout is 5230) | |||
There was a problem hiding this comment.
Maybe we can make it clearer what this number means?
e.g. (Waiting for async callback... will force flush in 5320 ms)
There was a problem hiding this comment.
I like that! Will fix.
gaearon
approved these changes
Mar 28, 2018
gaearon
approved these changes
Mar 29, 2018
This was referenced Apr 17, 2018
rhagigi
pushed a commit
to rhagigi/react
that referenced
this pull request
Apr 19, 2018
…ts (facebook#12479) * Update user timing to record the timeout deadline with 'waiting' events **what is the change?:** When we are processing work during reconciliation, we have a "timeout" deadline to finish the work. It's a safety measure that forces things to finish up synchronously if they are taking too long. The "timeout" is different depending on the type of interaction which triggered the reconciliation. We currently have a shorter "timeout" for "interactive updates", meaning we will try to finish work faster if the reconciliation was triggered by a click or other user interaction. For collecting more data in our logs we want to differentiate the 'waiting for async callback...' events based on the "timeout" so I'm adding that to the logging. One interesting note - in one of the snapshot tests the "timeout" was super high. Going to look into that. **why make this change?:** Right now we are debugging cases where an interaction triggers a reconciliation and the "waiting for async callback...' events are too long, getting blocked because the main thread is too busy. We are keeping logs of these user timing events and want to filter to focus on the reconciliation triggered by interaction. **test plan:** Manually tested and also updated snapshot tests. (Flarnie will insert a screenshot) * Improve wording of message * ran prettier
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
what is the change?:
When we are processing work during reconciliation, we have a "timeout"
deadline to finish the work. It's a safety measure that forces things to
finish up synchronously if they are taking too long.
The "timeout" is different depending on the type of interaction which
triggered the reconciliation. We currently have a shorter "timeout" for
"interactive updates", meaning we will try to finish work faster if the
reconciliation was triggered by a click or other user interaction.
For collecting more data in our logs we want to differentiate the
'waiting for async callback...' events based on the "timeout" so I'm
adding that to the logging.
One interesting note - in one of the snapshot tests the "timeout" was
super high.
Weird? Going to look into that.
why make this change?:
Right now we are debugging cases where an interaction triggers a
reconciliation and the "waiting for async callback...' events are too
long, getting blocked because the main thread is too busy. We are
keeping logs of these user timing events and want to filter to focus on
the reconciliation triggered by interaction.
test plan:
Manually tested and also updated snapshot tests.