-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Table CPU Performance Improvements #7392
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #7392 +/- ##
==========================================
- Coverage 56.02% 55.98% -0.05%
==========================================
Files 662 662
Lines 26302 26304 +2
Branches 2551 2551
==========================================
- Hits 14737 14727 -10
- Misses 10856 10865 +9
- Partials 709 712 +3
*This pull request uses carry forward flags. Click here to find out more.
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
If the throttle gets us an easy, measurable performance improvement, great! What we can really benefit from though is a restructuring of the Vue components surrounding the TelemetryTable (which is no small order).
The elephant in the room is that we pass the row
object all the way down to the TableCell
, so Vue cannot selectively re-render elements that change. Instead, it has to re-render the entire list every time a single change is made (a single row added or removed). See this recording I made with Vue's debugger "highlight on update" feature:
Screen.Recording.2024-01-23.at.10.51.44.AM.mov
This is a pattern that plagues Open MCT performance in some of our most used and most complex areas of the code, so it's going to be a project.
In short: I'm OK with this as a quick perf boost, but long-term we should not throttle but just restructure the components and let Vue do the heavy lifting.
Lots of good information here
I agree we should do the restructuring. I'm ok with keeping throttling long term as well, it's effectively batching updates and reducing the number of repaints which are always going to be costly. |
Closes
partial-close: #7268 (items 2, 3)
Describe your changes:
Two main changes here:
These changes resulted in a significant (100%+ -> 35%~) decrease in CPU usage.
All Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist