Force Curve UI Optimization

[DXCanas]

Problem

• Force curve chart redraws on every metrics update, even when data hasn't changed
• Root cause: Global state replacement pattern (appState = {...appState, newData}) invalidates all object references
• Chart.js updates are expensive

Solution

Single-property optimization using Lit's native features:

// Combined object property with pure hasChanged
@property({
  type: Object,
  hasChanged: (newVal, oldVal) => {
    // Short-circuit when flag is false - skip expensive comparison
    if (!newVal?.updateForceCurve) return false
    
    const newData = newVal?.value
    const oldData = oldVal?.value
    
    // Only do expensive array comparison when flag is on
    if (!oldData || newData?.length !== oldData?.length) return true
    return newData?.some((v, i) => v !== oldData[i])
  }
})
  forceCurveData = { updateForceCurve: false, value: [] }

Usage:

html`<dashboard-force-curve 
  .forceCurveData=${{ 
    updateForceCurve: metrics.metricsContext?.isRecoveryStart, 
    value: metrics?.driveHandleForceCurve 
  }}
/>`

• Also moved chart updates from render() to updated() per best practices
• Added JSDoc type annotations for better IDE support

Tradeoffs

• Requires passing combined object instead of separate properties
• Addresses symptom, not root cause (full state replacement pattern)

Benefits

• Pure hasChanged: No module-level state or side effects
• Immediate performance gain: O(n) array comparison only happens when needed
• Works with Lit's design: Uses documented lifecycle APIs as intended
• Cleaner API: Single property instead of coordinated pair
• Isolated change: No refactoring of state layer required
• Stepping stone: Compatible with future state management improvements

Testing

See comments for detailed logs

---

Commits pulled out from #170 to unblock it.

Shoutout to @Abasz for the updateForceCurve property. Cherry-picked and integrated the commit from his lit upgrade branch (which I believe is 0.9.8 targetted)

Discussion:

#173 (reply in thread)
#173 (comment)
99bf915
#170 (comment)
#168 (reply in thread)
#168 (reply in thread)
#168 (reply in thread)

[DXCanas]

Not a substitute for a state refactor @Abasz @JaapvanEkris

Just an incremental step

[DXCanas]

A little hard to follow, and I'm using the simulated row commented out in server.js.

Sampling with changes, from first render/updated call:

[ForceCurve] hasChanged #91 @ 1769927281950: updateForceCurve=false → FALSE (skipped, no deep check) (total: 0T/91F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #92 @ 1769927282098: updateForceCurve=true, len 0→69, contentChanged=false → TRUE (total: 1T/91F)
index-DCVBrq_V.js:174 [ForceCurve] render() #3 @ 1769927282099, elapsed since last: 17555ms
index-DCVBrq_V.js:174 [ForceCurve] updated() #1 @ 1769927282099, elapsed since last: N/Ams, dataPoints: 69, updateForceCurve=true
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #93 @ 1769927282299: updateForceCurve=false → FALSE (skipped, no deep check) (total: 1T/92F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #94 @ 1769927282499: updateForceCurve=false → FALSE (skipped, no deep check) (total: 1T/93F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #95 @ 1769927282700: updateForceCurve=false → FALSE (skipped, no deep check) (total: 1T/94F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #96 @ 1769927282751: updateForceCurve=false → FALSE (skipped, no deep check) (total: 1T/95F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #97 @ 1769927282953: updateForceCurve=false → FALSE (skipped, no deep check) (total: 1T/96F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #98 @ 1769927283155: updateForceCurve=false → FALSE (skipped, no deep check) (total: 1T/97F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #99 @ 1769927283354: updateForceCurve=false → FALSE (skipped, no deep check) (total: 1T/98F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #100 @ 1769927283433: updateForceCurve=true, len 69→5, contentChanged=false → TRUE (total: 2T/98F)
index-DCVBrq_V.js:174 [ForceCurve] render() #4 @ 1769927283433, elapsed since last: 1334ms
index-DCVBrq_V.js:174 [ForceCurve] updated() #2 @ 1769927283433, elapsed since last: 1334ms, dataPoints: 5, updateForceCurve=true
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #101 @ 1769927283633: updateForceCurve=false → FALSE (skipped, no deep check) (total: 2T/99F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #102 @ 1769927283834: updateForceCurve=false → FALSE (skipped, no deep check) (total: 2T/100F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #103 @ 1769927284036: updateForceCurve=false → FALSE (skipped, no deep check) (total: 2T/101F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #104 @ 1769927284065: updateForceCurve=false → FALSE (skipped, no deep check) (total: 2T/102F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #105 @ 1769927284266: updateForceCurve=false → FALSE (skipped, no deep check) (total: 2T/103F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #106 @ 1769927284468: updateForceCurve=false → FALSE (skipped, no deep check) (total: 2T/104F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #107 @ 1769927284669: updateForceCurve=false → FALSE (skipped, no deep check) (total: 2T/105F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #108 @ 1769927284869: updateForceCurve=false → FALSE (skipped, no deep check) (total: 2T/106F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #109 @ 1769927285071: updateForceCurve=false → FALSE (skipped, no deep check) (total: 2T/107F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #110 @ 1769927285272: updateForceCurve=false → FALSE (skipped, no deep check) (total: 2T/108F)
index-DCVBrq_V.js:174 [ForceCurve] hasChanged #111 @ 1769927285397: updateForceCurve=true, len 5→57, contentChanged=false → TRUE (total: 3T/108F)
index-DCVBrq_V.js:174 [ForceCurve] render() #5 @ 1769927285397, elapsed since last: 1964ms
index-DCVBrq_V.js:174 [ForceCurve] updated() #3 @ 1769927285397, elapsed since last: 1964ms, dataPoints: 57, updateForceCurve=true

[JaapvanEkris]

Dumb question: why compare the force curve to begin with? The rowing engine will only update the force curve at the end of the drive (so the metricscontext's isRecoveryStart is true). Wouldn't that be much simpler?

[Abasz]

I am also reading the docs: https://lit.dev/docs/components/lifecycle/#willupdate and some tutorials on best practices: https://open-wc.org/blog/some-things-to-know-when-developing-with-lit/

Based on the lifecycle hook I think It was a bead practice to put the chart data update in to render (though from a practical perspective the result is the same) and should have been put be moved to the willUpdate() method.

I think it is recommended that render does generally create side effects. Though strictly speaking I dont see much difference but semantically it may be relevant as it better communicates intent?

willUpdate() does not return a value and does not require super() call which tells me that it is specifically for component internal side effects existing outside its life and change detection flow.

Dumb question: why compare the force curve to begin with? The rowing engine will only update the force curve at the end of the drive (so the metricscontext's isRecoveryStart is true). Wouldn't that be much simpler?

I would leave that in just for defence against future changes (lets be honest on rpi this is rather a micro optimisation as it is not too expensive on a 50-100 element array, with full honestly I realize now that when I made this comment I was more in a micro controller mindset...) for now.

Actually i have been looking at Lit "Data Management" and that will solve many of these problems. Once I gathered enough information on best practices I will sketch up something but the main idea is that with Lit Context we can inject data service directly into a component without the need for passing down everything from parent to child. And I am hoping that the individual metric component from there can take the array reference (via exposed readonly signal) held by the data service which will have proper scheduling for updating these properties based on metrcis context (which was the original intention anyway). This will properly encapsulate things, protect the data from outside changes as well as allocate the update scheduling to one place and we have full control.

I dont like the idea of the server doing this, unless we create separate messages for the web socket (which would be similar to the PM5 specification) so for instance we fire a message recoveryStart and we include everything that changes, we fire a separate event for continuous metrics (distance for instance) etc. But in all honesty this will not change the overall complexity in a good way as we would need to have logic in web server to make decision what WS message to fire, and then another logic in the client how to handle the different messages. If we keep webserver as is and we have the logic in client it will not require doubling anything (client will need this anyway).

Furthermore, I generally like those APIs where you expose a set of data that is very very stable and then leave clients to deal with them how ever they want. Its read only, webserver is still single source of truth, its relatively light weight (I mean we can of course save some bandwidth if we split the broadcast on the server as we would not send handle force array with every packet, but this is again a micro optimisation for the RPi, when I was doing ESPRM with web socket I did this because I wanted to limit the packet sizes as much as possible for the 240Mhz processor - and at the time I have not dared to use the second core for this :) so wifi communication was on the same core as the main ISRs and that messed with interrupts in rare cases... looking back to that it was not so mature implementation, but any way).

[DXCanas]

why compare the force curve to begin with?

That actually how the initial implementation; the array was always different because the parents' object was always "different" (A shallow copy might actually be like. nice. in this case.)? So the initial performance issue was that change was being triggered component wide every single time the websocket gave us new data. A deep equality would ensure that change was only triggered when the data actually changed.

It was important to me that we start adhering to the few patterns that lit suggests, because they align with other, more opinionated frameworks' in case of a future switch (and also just… legibility).

So I implemented hasChanged as the fix. Which worked! but then @Abasz, in one of the comments in the description, noted that that was computationally expensive and brought in the flag in a separate property.

… I thought it was a kind of a pre-optimzation, where O(n) ain't too bad in a modern browser (or even O(2n)). Especially speaking relative to what was happening before. But y'all are the bosses!

And this makes sense of it to me now:

with full honestly I realize now that when I made this comment I was more in a micro controller mindset...

The only benefit I can think of for keeping it is the freakish case where an individual somehow draws the exact same curve 🤷

---

After looking more closely at the logs, I agree with this sentiment:

should have been put be moved to the willUpdate() method.

---

Once I gathered enough information on best practices I will sketch up something but the main idea is that with Lit Context we can inject data service directly into a component without the need for passing down everything from parent to child.

^ I don't think Context is a bad idea. But I do think there are smaller changes partways through. Whether they're worth it is something worth talking about, but uh. Probably not here. Some discussion of this over in #175. I agree with the comment after that point, but would rather have the discussion elsewhere for the same reason 😄

---

After the review over in #170, I have some other ideas, though! Will update both!

[DXCanas]

why compare the force curve to begin with? The rowing engine will only update the force curve at the end of the drive (so the metricscontext's isRecoveryStart is true). Wouldn't that be much simpler?

I would leave that in just for defence against future changes

Funnily enough, after throwing in more logs and letting it run, it looks like it's the array length comparison that gets evaluated most often 🤷 idk if this will be the case in real user practice, though:

13 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called
15 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called
16 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called
7 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called
9 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called
13 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: array deep compare result  true
 render called
 updated called
7 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called
8 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called
7 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called
10 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called
15 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called
10 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called
10 NO CHANGE:updateForceCurve is false
 updateForceCurve is true, proceeding
 CHANGE: different length
 render called
 updated called

But yeah, alternative which I think is most closely aligned to @Abasz's original concept: split the props back up and then basically ignore all changes to the force curve array (hasChanged always returns false).

I think that'd basically be the most performant.

Initially, I wondered if a singleupdateForceCurve: true would amount to multiple updates, but confirmed that is not the case with logs and then thought about it a bit more and… realized that was not possible with the way the lifecycle works 🫠

[JaapvanEkris]

Dumb question: why compare the force curve to begin with? The rowing engine will only update the force curve at the end of the drive (so the metricscontext's isRecoveryStart is true). Wouldn't that be much simpler?

I would leave that in just for defence against future changes (lets be honest on rpi this is rather a micro optimisation as it is not too expensive on a 50-100 element array, with full honestly I realize now that when I made this comment I was more in a micro controller mindset...) for now.

But this is one of the key elements of the current architecture: the collection of flags is there by design to help data consumers understand the data and trigger updates. And it guarantees that everybody displays the same data regardless of channel (so that the force curve on your monitor will be the same as displayed in EXR or ErgZone and recorded by the RowsAndAll recorder). No need for local comparisons and half-baked guesses: the metricsContext will tell what state the rowing machine is. All recorders and peripherals use the exact same logic (the RowsAndAll recorder even uses it to trigger recording of strokes with the same force curve) and report the same data.

[DXCanas]

But this is one of the key elements of the current architecture: the collection of flags is there by design to help data consumers understand the data and trigger updates. And it guarantees that everybody displays the same data regardless of channel

Makes sense to me, and aligns with Abasz's original rationale for including the change 🙂; I've reverted the previous change. Both the curve array and the flag bool are taken in, but the flag is the only thing that triggers updates. I think this is the most pattern-compliant way to do it in lit with the way the data layer currently is.

willUpdate() does not return a value and does not require super() call which tells me that it is specifically for component internal side effects existing outside its life and change detection flow.

• willUpdate() now only prepares the data (following Lit's guidance for computing values needed for rendering)
• updated() handles the chart.update() call (since it runs after DOM rendering is complete, and I think this is what canvas-manipulating libraries expect. Trying to cash in on any potential optmizations on the chart.js side.)

[DXCanas]

Actually, something interesting @JaapvanEkris

While I agree with your point architecturally, the former approach (object with array and bool) is resulting in less renders according to the logging I've got going

In its current state, using only the flag and no deep checks, it's actually exhibiting and odd "staggered"/"double-tap" render every change.

The bool flipping from true -> false is one render
then the bool flipping from false -> true is another

The last approach, where both the bool and the array were checked made it so that renders happened half as often, with a pretty performant array length check "winning out" most of the time.

Update:

I've found another way, that I think y'all will like better! Saw it while going through lifecycle docs, very aptly named (shouldUpdate).

[DXCanas]

Marking as draft until we merge #170 , as there are merge conflicts now 🫠

[DXCanas]

Merge conflicts resolved!