Warn when suspending at wrong priority#15492
Merged
acdlite merged 2 commits intofacebook:masterfrom May 7, 2019
Merged
Conversation
Details of bundled changes.Comparing: 89d8d14...95990bb react-dom
react-art
react-native-renderer
react-test-renderer
react-reconciler
Generated by 🚫 dangerJS |
acdlite
force-pushed
the
warn-suspense-priority
branch
2 times, most recently
from
07911d3 to
162af79
Compare
sebmarkbage
reviewed
Apr 26, 2019
| // | ||
| // However, we do store the last discrete pending update per root. So we | ||
| // can reliably compare to that one. (If we broaden this warning to include | ||
| // high pri updates that aren't discrete, then this won't be sufficient.) |
Contributor
There was a problem hiding this comment.
We really should have this warning too since it's pretty important to fix the mousemove events to have this property and we really shouldn't suspend there.
Collaborator
Author
There was a problem hiding this comment.
Any thoughts on implementation then? I could track the high pri times per root in dev.
Contributor
There was a problem hiding this comment.
We can get back to it later. I suspect I'll need to track some new field for suspendIfNeeded. Maybe we can use that.
Adds a warning when a user-blocking update is suspended. Ideally, all we would need to do is check the current priority level. But we currently have no rigorous way to distinguish work that was scheduled at user- blocking priority from work that expired a bit and was "upgraded" to a higher priority. That's because we don't schedule separate callbacks for every level, only the highest priority level per root. The priority of subsequent levels is inferred from the expiration time, but this is an imprecise heuristic. However, we do store the last discrete pending update per root. So we can reliably compare to that one. (If we broaden this warning to include high pri updates that aren't discrete, then this won't be sufficient.) My rationale is that it's better for this warning to have false negatives than false positives. Potential follow-ups: - Bikeshed more on the message. I don't like what I landed on that much but I think it's good enough to start. - Include the names of the components that updated. (The ideal place to fire the warning is during the setState call but we don't know if something will suspend until the next update. Maybe we could be clever and warn during a subsequent update to the same component?)
acdlite
force-pushed
the
warn-suspense-priority
branch
from
162af79 to
0e8acda
Compare
acdlite
force-pushed
the
warn-suspense-priority
branch
from
0e8acda to
95990bb
Compare
sebmarkbage
approved these changes
May 7, 2019
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Adds a warning when a user-blocking update is suspended.
Ideally, all we would need to do is check the current priority level. But we currently have no rigorous way to distinguish work that was scheduled at user-blocking priority from work that expired a bit and was "upgraded" to a higher priority. That's because we don't schedule separate callbacks for every level, only the highest priority level per root. The priority of subsequent levels is inferred from the expiration time, but this is an imprecise heuristic.
However, we do store the last discrete pending update per root. So we can reliably compare to that one. (If we broaden this warning to include high pri updates that aren't discrete, then this won't be sufficient.)
My rationale is that it's better for this warning to have false negatives than false positives.
Potential follow-ups: