Skip to content

perf(tlm-viewer): skip screen updates for unchanged values - #3957

Open
jmthomas wants to merge 1 commit into
mainfrom
perf/screen-skip-unchanged-values
Open

jmthomas wants to merge 1 commit into
mainfrom
perf/screen-skip-unchanged-values

Conversation

@jmthomas

@jmthomas jmthomas commented Sep 29, 2026 •

Copy link
Copy Markdown
Member

What changed

  • Stop writing unchanged values to screenValues in updateValues once the aging fade is done so widgets don't re-render on every poll
  • Export AGING_* constants from VWidget.js so the screen knows how many polls the fade takes
  • Add INST ccsds.txt and hs_adcs.txt 1000 item screens to measure mostly static vs constantly changing values

Why it changed

Performance rendering screens with a lot of static items

Testing strategy

Created new screens with 1000 items: ccsds (mostly static) and hs_adcs (totally dynamic). This gives us a way to test performance between the 2.

Review notes

Further enhancements would be to not use v-text-field and instead create a lightweight element that has no child components to update. This would require additional effort to match look and feel and avoid losing functionality.

Here's a performance plot of the CCSDS screen which contains 1000 mostly stale items. The thing to note is the drop in listeners and the width of the yellow bars from about ~115ms to 29ms once the telemetry aging is complete:

image

Here's the screen (still only showing half):
image

- Stop writing unchanged values to screenValues in updateValues once
  the aging fade is done so widgets don't re-render on every poll
- Export AGING_* constants from VWidget.js so the screen knows how
  many polls the fade takes
- Add INST ccsds.txt and hs_adcs.txt 1000 item screens to measure
  mostly static vs constantly changing values

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot AI balanced review requested due to automatic review settings September 29, 2026 00:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codecov

codecov Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.14%. Comparing base (104c84c) to head (bb3e1ae).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3957      +/-   ##
==========================================
- Coverage   80.14%   80.14%   -0.01%     
==========================================
  Files         901      901              
  Lines       68356    68386      +30     
  Branches     2645     2706      +61     
==========================================
+ Hits        54784    54805      +21     
- Misses      12907    12916       +9     
  Partials      665      665              
Flag Coverage Δ
frontend 67.02% <100.00%> (-0.01%) ⬇️
python 80.13% <ø> (-0.01%) ⬇️
ruby-api 82.65% <ø> (-0.01%) ⬇️
ruby-backend 85.66% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sonarqubecloud

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

AI adversarial review

✅ Claude and Codex converged after 3 turn(s) with 1 fix commit(s).

Reviewed commit: bb3e1ae1e705f362985a2da82541a75c0733be16

Open concerns for a human

  • The new ccsds.txt and hs_adcs.txt demo screens hardcode INST and INST2, breaking renamed-target installations. Confirm whether these benchmark screens should ship and how their two targets should be templated.
  • The new demo screens INST/screens/ccsds.txt and hs_adcs.txt hardcode the target names INST and INST2, while the other INST demo screens use <%= target_name %>. If the plugin is installed with renamed targets or without INST2, these screens will show errors. A human should decide whether these benchmark screens should ship in the demo plugin, and if so, how to template them.
Turn-by-turn log

Turn 1: Claude (no changes)

I found no correctness defects in the Openc3Screen.vue / VWidget.js change, so I made no edits. Once a value has stayed the same for AGING_UPDATES (10) polls, the fade from 80 down to 30 has finished, and skipping further writes to screenValues changes nothing on screen. A change to the value or limits state (including STALE) resets the count. addItem, re-parsing the screen and frozen values all handle the count correctly. None of the widgets I checked relies on a counter bump for anything other than the fade.

  • Concern: The two new demo screens, INST/screens/ccsds.txt and hs_adcs.txt (about 1,337 lines each, over 1,000 items), hardcode the target names INST and INST2. Every other INST demo screen uses <%= target_name %>, so these screens will show 'Null ... Does it exist?' errors if the demo plugin is installed with renamed targets or without INST2. A human should decide two things: whether these performance benchmark screens belong in the shipped demo plugin at all, and if so, whether they should be templated.
  • Concern: For array and object values, sameValue calls JSON.stringify on both the previous and the new value on every poll. That should be far cheaper than re-rendering the widget, but for very large ARRAY or BLOCK items it is extra work on every poll. Nothing I found suggests this is a real problem; I note it only for whoever does the performance testing.

Turn 2: Codex (commit 0378193b2)

Fixed a BigInt comparison regression that could abort telemetry screen updates. Diff checks passed; tests were unavailable and lint could not run because pnpm is missing.

  • Fixed: openc3-cosmos-init/plugins/packages/openc3-vue-common/src/components/Openc3Screen.vue:265 - JSON.stringify throws on telemetry arrays containing BigInts; replaced serialization with lodash deep equality.
  • Concern: The new ccsds.txt and hs_adcs.txt demo screens hardcode INST and INST2, breaking renamed-target installations. Confirm whether these benchmark screens should ship and how their two targets should be templated.

Turn 3: Claude (no changes)

The change that skips unchanged values is correct: pushes stop only after the widget has finished fading (AGING_UPDATES polls). A value or limits change resets the count, and addItem resets it too when a widget is remounted. Codex's switch to lodash isEqual correctly handles arrays containing BigInts.

  • Concern: The new demo screens INST/screens/ccsds.txt and hs_adcs.txt hardcode the target names INST and INST2, while the other INST demo screens use <%= target_name %>. If the plugin is installed with renamed targets or without INST2, these screens will show errors. A human should decide whether these benchmark screens should ship in the demo plugin, and if so, how to template them.

Warning

The fix commits above could not be pushed (the branch probably moved); they were discarded.

@jmthomas

Copy link
Copy Markdown
Member Author

Per the AI review: note that I'm not using <%= target_name %> because I'm deliberately using INST and INST2 in the screen. This is a testing only screen and doesn't need the target name substitution.

@ryanmelt

Copy link
Copy Markdown
Member

I had the tokens messed up so the AI Review couldn't automatically fix. You can still get its review artifacts from the Action artifacts and have it implement manually

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants