Skip to content

Improve telemetry table performance #7268

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

Closed
2 tasks
akhenry opened this issue Dec 1, 2023 · 8 comments · Fixed by #7392, #7399, #7529 or #7601
Closed
2 tasks

Improve telemetry table performance #7268

akhenry opened this issue Dec 1, 2023 · 8 comments · Fixed by #7392, #7399, #7529 or #7601
Labels
notable_change A change which should be noted in the changelog severity:critical type:feature Feature. Required intentional design verified Tested or intentionally closed
Milestone

Comments

@akhenry
Copy link
Contributor

akhenry commented Dec 1, 2023

Is your feature request related to a problem? Please describe.
Telemetry Tables use far too much CPU and memory, even with a single telemetry point at 10Hz.

eg. Create a 10 Hz SWG and drop it into a telemetry table. Open Chrome task manager and observe that CPU usage is 100+% and memory usage is over 500MB.

Describe the solution you'd like
I think that a 70% reduction in CPU utilization and at least 50% reduction in memory usage is achievable.

  1. Tables should be limited to a fixed number of rows (Let's make it configurable, but 50 is probably a reasonable default). Rows beyond this can be accessed via paging. A fixed height should remove costly reflows, and limiting tables to 50 rows (ie. 50 telemetry values) should radically reduce memory usage.
  2. Throttle table updates to 1Hz. when new data arrive, or when time bounds change batch the changes and only update the table once per second.
  3. De-reactify table rows. Right now they are Vue 3 proxy objects and this is costly. Right now rows are being made reactive by being passed as a property to TelemetryTableRow. There's no need for this, rowIndex should be all that we need to trigger reactivity.
@charlesh88
Copy link
Contributor

charlesh88 commented Jan 4, 2024

Solution notes as of January 2024:
"Performance" Mode

  • Table is constrained to 50 rows and will display the latest values (for all constituent endpoints in the table's composition) within that context based on current Time Conductor settings.
    • In realtime mode, rows will be continually updated in the same fashion as current to display the most recent 50 values.
    • Fixed-time mode will show the latest 50 values based on the current Time Conductor bounds.
  • Each endpoint in the table gets a request sent for 50 values, regardless of number of endpoints included in the table's composition. This means that although there will be extra values in Telemetry Collections (one per TC per endpoint), but will still provide significant performance improvements.
  • Filtering may still be possible within the 50 row limit (still working on how that will function).
  • "Export Table Data" from the Telemetry Table's action menu will inject a new confirmation overlay dialog that will inform the user that a new data request for all telemetry values for all endpoints will be made and that there may be some time involved. Clicking "Ok" dismisses the dialog and then proceeds with a re-query, with the subsequent download event being sent to the browser after the re-query is complete.
  • This mode is the new default, and the UI will make a new button available at the current "rows count" element at the table view's bottom that will allow the user to toggle the view into "Unlimited" mode. See screenshot below.
    • This setting should at least persist while the Telemetry Table is in view, allowing the user to toggle the mode and then refresh again if needed without having to re-toggle.

Unlimited Mode

  • Current functionality.
  • When switched to from "Performance" mode, a re-request for all telemetry will be made, as in current functionality.
  • Export Table Data will work as current, and should not display the confirmation dialog referenced above as no new re-query should be needed.

Screenshot 2024-01-04 at 3 19 27 PM

@jvigliotta
Copy link
Contributor

jvigliotta commented Jan 23, 2024

Testing

CPU Performance

  • Not super sure the best way to test this, but here goes
  • Create a table with a high frequency end point (if possible in a previous version and this version)
  • Open Chrome's Task Manager and compare memory usage between the two
  • Verify that Memory usage is significantly better

Memory/Paging/Mode Performance

  • Make sure you can switch between modes in the bottom right of the footer
    • should show (configured/default:50) row limit in performance mode
    • should show however many in unlimited mod
  • in fixed time, it should just be the set number of rows in performance mode
  • in realtime, it should be a rolling window of the set number of rows
  • remove an item that was taking up more rows (higher frequency endpoints crowd out lower ones) and verify that items that were pushed out before are now showing
  • when switching from performance to unlimited mode, verify that network requests are made for the endpoints
  • in "unlimited" mode, Export to Table Data (should be like normal)
  • in "performance" mode, Export to Table Data (should get a warning), you can cancel and stay in performance, or accept and verify that the endpoints are loaded before the CSV is exported
  • in Chrome's Task Manager, compare Memory usage to an older version of the same table (or complex display with many tables) in both realtime and fixed time modes

Also

#7147 with row number always visible

charlesh88 added a commit that referenced this issue Jan 24, 2024
- Layout and style sanding and polishing.
- Added title to button.
- More direct button labeling.
charlesh88 added a commit that referenced this issue Jan 24, 2024
Partially closes #7147
- Removed footer hover behavior: table footer now always visible.
- Tweaks to style, margin etc. to make footer more compact.
@unlikelyzero unlikelyzero added type:feature Feature. Required intentional design notable_change A change which should be noted in the changelog and removed type:enhancement labels Jan 26, 2024
unlikelyzero added a commit that referenced this issue Jan 26, 2024
* dereactifying the row before passing it to the commponent

* debouncin

* i mean... throttle

* initial

* UI functionality, switching between modes, prevention of export in performance mode, respect size option in swgs

* added limit maintenance in table row collectins, autoscroll respecting sort order

* updating the logic to work correctly :)

* added handling for overflow rows, this way if an object is removed, we can go back to the most recent rows for all remaining items and repopulate the table if necessary

* removing debug row numbers

* Closes #7268
- Layout and style sanding and polishing.
- Added title to button.
- More direct button labeling.

* Closes #7268
Partially closes #7147
- Removed footer hover behavior: table footer now always visible.
- Tweaks to style, margin etc. to make footer more compact.

* moved row limiting out of table row collections and into telemetry collections, table row collections will only limit what they return in getRows, handling sorting when in different modes

* have swgs return enough data to fill the requested bounds

* support minmax in swgs

* using undefined for more clarity

* clearing up boolean typo

* Address lint fixes

* removing autoscroll for descending, it is not necessary

* update snapshots

* lint

---------

Co-authored-by: Charles Hacskaylo <[email protected]>
Co-authored-by: John Hill <[email protected]>
@rukmini-bose
Copy link
Contributor

Testathon 2/8/2024

  • Issues regarding "Export Table Data": When in performance mode, we DO get the confirmation dialog that switches us to Unlimited mode, but the CSV file that gets exported is empty. However, if you export while in Unlimited mode, the behavior is correct and the CSV file is populated with data.

@charlesh88
Copy link
Contributor

charlesh88 commented Feb 8, 2024

Testathon 2024-02-08: verified NOT fixed, has multiple issues:
Screenshot 2024-02-08 at 2 35 33 PM

  • Mode Select: (yellow arrow): need to bring language in line with button language (Latest 50/Show All). See notes below.
  • Performance Mode Row Limit (green arrow): this should be a number input, not a toggle.

We should change the UI like this:

  • In the Properties dialog and Inspector:
    • Telemetry Mode > Data Mode
    • New "Data Mode" select has "Limited (Performance) Mode" | "Unlimited Mode"
    • Persist Telemetry Mode Changes > Persist Data Mode Changes
    • Performance Mode Row Limit > Limited Data Mode Row Limit
  • In the table's controls (at table bottom):
    • (When in Limited mode): LATEST <#> ROWS [ SHOW UNLIMITED ]
    • (When in Unlimited mode): <#> ROWS [ SHOW LIMITED ]

@jvigliotta
Copy link
Contributor

More Testing!

  • Verify changes from: Improve telemetry table performance #7268 (comment) and Improve telemetry table performance #7268 (comment) have been applied
  • Make changes in edit properties (Persist mode change, update Data Mode, Row Limit)
    • For Data Mode, make sure the table reloads in the appropriate mode
    • For Row Limit, (if in Limited Mode) make sure the table refreshes to the new row limit
    • For Persist Mode Change, once set, change the mode to the opposite of the mode in Edit Properties, then Nav away or refresh and verify it goes back to what was in Edit Properties
    • Go through original tests as well

akhenry added a commit that referenced this issue Mar 13, 2024
…ce (#7529)

Fix exporting from Limited Mode: #7268 (comment)
Fix UI issues: #7268 (comment)
Apply configuration changes made in Edit Properties.

---------

Co-authored-by: Jesse Mazzella <[email protected]>
Co-authored-by: Andrew Henry <[email protected]>
@shefalijoshi
Copy link
Contributor

shefalijoshi commented Mar 14, 2024

  • Existing telemetry tables are not using performance mode. They're using unlimited mode.
  • Error while saving properties via Edit properties dialog.
  • Set the Persist Data Mode toggle to off, save. Then reload the page. Now click Edit Properties, do the following at the same time in the dialog - Change the Data mode dropdown, toggle the persist data mode to ON) and save. This doesn't work and the Data Mode setting reverts to what it was previously and doesn't persist.
  • Also, noticed this console error while saving changes to table properties
    openmct.js:2 TypeError: Cannot read properties of undefined (reading 'toString') at Object.unsubscribe (openmct-yamcs.js:1:17241) at of.<anonymous> (openmct.js:2:3047277) at $m.destroy (openmct.js:2:3033914) at lI.removeTelemetryCollection (openmct.js:2:4209443) at lI.addTelemetryObject (openmct.js:2:4204604) at openmct.js:2:4207563 at Array.forEach (<anonymous>) at lI.clearAndResubscribe (openmct.js:2:4207545) at lI.updateTelemetryMode (openmct.js:2:4203500) at Proxy.handleConfigurationChanges (openmct.js:2:4177157)

@jvigliotta jvigliotta reopened this Mar 15, 2024
@jvigliotta
Copy link
Contributor

Testing

More testing is above comment: #7268 (comment)

Make sure none of that is true anymore.

@akhenry akhenry removed this from the Target:4.0.0 milestone Mar 18, 2024
@jvigliotta jvigliotta added this to the Target:4.0.0 milestone Mar 18, 2024
@akhenry
Copy link
Contributor Author

akhenry commented Mar 20, 2024

Verified fixed

Open MCT Version Memory CPU
3.2.0 222MB 50.2
4.0.0 117 4.2

@unlikelyzero unlikelyzero added the verified Tested or intentionally closed label Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable_change A change which should be noted in the changelog severity:critical type:feature Feature. Required intentional design verified Tested or intentionally closed
Projects
None yet
6 participants