Chart updates to improve stability, possible fix for infinite loop #18329
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
bramkragten
reviewed
Oct 24, 2023
| if (this._paddingUpdateCount > 300) { | ||
| this._paddingUpdateLock = true; | ||
| // eslint-disable-next-line | ||
| console.warn( |
There was a problem hiding this comment.
Suggested change
| console.warn( | |
| console.error( |
There was a problem hiding this comment.
Maybe we should just throw, and log in the core log so people can report it...
bramkragten
reviewed
Oct 24, 2023
| this._paddingYAxisInternal = this.paddingYAxis; | ||
| if (changedProps.size === 1 && changedProps.has("paddingYAxis")) { | ||
| this._paddingUpdateCount++; | ||
| if (this._paddingUpdateCount > 300) { |
There was a problem hiding this comment.
If this happens in 2 secs, it is pretty bad, we should fix that 😓
bramkragten
approved these changes
Oct 24, 2023
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.
Proposed change
Two updates to history charts to attempt to improve rendering stability:
First is an update to not recompute start/end time in the
renderfunction of state-history-charts, as render is called after a child chart fires a y axis width change. That event should trigger a re-render of the children with different axis padding value, but it should not functionally change the data in the chart. I think this continual re-updating of time ranges and therefore data on each re-render can sometimes contribute to infinite render loops.Second change is an escape hatch in the chart base that tries to detect if it caught in an infinite loop of rerendering layout padding changes (as has happened a few times in the past, when chart.js gets into an oscillation where it keeps flipping back and forth between two different axis width values on each render), and if it does, shows a warning and stops trying to maintain axis alignment to break the infinite loop, I think this could potentially prevent a hard browser hang. These cases are often difficult to predict and difficult to reproduce, sometimes depending on the time of day due to how chart.js lays out the tick labels. After making the first change above I am not currently able to trigger this case in my dev instance, but it may be helpful to speculatively prevent future issues, versus letting browser hang occur and trying to debug afterward.
I have a pathological test view with dozens of charts of varying options in various grid sizes, and without these changes I am occasionally (but not always) able to trigger a hard lockup in the browser when aggressively resizing the window. After making these changes I am not able to trigger a hang, so they seem to be helping.
Type of change
Example configuration
Additional information
Checklist
If user exposed functionality or configuration variables are added/changed: