Skip to content

Optimize DataStore versioning and reduce allocations#68

Merged
haoruizhou merged 1 commit into
mainfrom
pecan-fixes
May 21, 2026
Merged

Optimize DataStore versioning and reduce allocations#68
haoruizhou merged 1 commit into
mainfrom
pecan-fixes

Conversation

@haoruizhou
Copy link
Copy Markdown
Contributor

Add a monotonic _version counter to DataStore (getVersion) and increment it on all mutating operations so subscribers can skip redundant work. Reduce allocations when rounding sensor readings by only cloning signal objects if rounding changes the value (preserving identity for common cases). Update useAllLatestMessages to snapshot the version and avoid unnecessary Map rebuilds, and batch cold-store state into a single object to cut re-renders. Simplify Trace memo usage that was a no-op. Add tests for version tick and ingestMessage data handling (rounding, immutability, defaults). Files changed: DataStore.ts, useDataStore.ts, Trace.tsx, and DataStore.test.ts.

@haoruizhou
Copy link
Copy Markdown
Contributor Author

@codex

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 07bd4107cc

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread pecan/src/lib/DataStore.ts Outdated
Comment on lines +617 to +618
this.notifyAll(msgID);
this._version++;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Move the version bump before notifying subscribers

When a live frame is ingested after useAllLatestMessages has subscribed, the subscriber runs during notifyAll while getVersion() still returns the old value, so the hook's currentVersion !== lastVersion guard skips rebuilding the latest-message Map. A dashboard that is mounted before the first/only CAN frame will not show it until a later write, and live data is always one notification behind; the same ordering appears in batch/clear paths, so the version needs to be incremented before notifying.

Useful? React with 👍 / 👎.

Comment on lines +572 to +574
roundedData[key] = rounded === signal.sensorReading
? signal
: { ...signal, sensorReading: rounded };
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Copy signals even when rounding is a no-op

For any signal whose reading is already rounded, the stored sample now keeps the caller's signal object by reference. If the decoded message object is reused or mutated after ingestMessage, historical data in DataStore changes as well; the previous implementation always spread each signal and the new immutability test only covers the changed-rounding case, so no-op values still alias the caller.

Useful? React with 👍 / 👎.

@haoruizhou haoruizhou requested a review from Ellaharding1 May 13, 2026 17:06
@haoruizhou
Copy link
Copy Markdown
Contributor Author

@Ellaharding1 can you review these optimizations please, thank you

@Ellaharding1
Copy link
Copy Markdown
Contributor

@haoruizhou just a thought on this since _version is incremented after notifyAll, I wonder if the version check in the subscriber callback might always see the old value and skip the update? Could potentially mean live data doesn't come through. Might be worth swapping those two lines just to be safe

…version before notifyAll so subscribers see correct version.
@haoruizhou haoruizhou merged commit 3d29966 into main May 21, 2026
23 checks passed
@haoruizhou haoruizhou deleted the pecan-fixes branch May 21, 2026 14:17
haoruizhou added a commit that referenced this pull request May 21, 2026
…version before notifyAll so subscribers see correct version. (#68)
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.

2 participants