Skip to content

fix: compare indicator figures by value in shouldUpdate - #839

Open
NemeZZiZZ wants to merge 1 commit into
klinecharts:mainfrom
NemeZZiZZ:fix/indicator-should-update-figures
Open

fix: compare indicator figures by value in shouldUpdate#839
NemeZZiZZ wants to merge 1 commit into
klinecharts:mainfrom
NemeZZiZZ:fix/indicator-should-update-figures

Conversation

@NemeZZiZZ

Copy link
Copy Markdown
Contributor

Problem

Default Indicator.shouldUpdate compares figures by reference:

const calc = JSON.stringify(prev.calcParams) !== JSON.stringify(current.calcParams)
  || prev.figures !== current.figures   // ← always true
  || prev.calc !== current.calc

override() builds _prevIndicator via clone() (typeChecks.ts — recursive deep copy), so prev.figures is always a different array instance than current.figures. The reference comparison is therefore unconditionally true, making calc === true — and since draw = calc || …, also draw === true — on every overrideIndicator call.

Verified by execution against the real source (Node, verbatim Indicator.ts + typeChecks.ts): a no-op overrideIndicator({ id }) returns calc: true, and so does overrideIndicator({ id, visible: false }).

Impact

Any overrideIndicator — toggling visible, changing zLevel, moving to another pane, restyling — triggers a full calc(dataList, indicator) over the entire history plus regenerateFigures and a full layout, where a redraw would suffice. On 50k bars with several indicators this is hundreds of milliseconds of pure waste per call.

Fix

Compare figures by value, the same way calcParams on the same line already is:

JSON.stringify(prev.figures) !== JSON.stringify(current.figures)

One line. figures arrays are small (a handful of figure descriptors), so the stringify cost is negligible against a full indicator recalc.

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.

1 participant