-
Notifications
You must be signed in to change notification settings - Fork 4
Add versioned cache invalidation and materialize() to Series for production streaming use cases #66
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
Conversation
- Add BarData class with version tracking for automatic cache invalidation - Update Series to use BarData and track cache version - Add materialize() method to break closure chains and free memory - Maintain backward compatibility with Bar[] arrays - Add comprehensive test suite with 33 tests covering all new features Co-authored-by: deepentropy <8287111+deepentropy@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds production-ready cache invalidation and memory management to the Series class for streaming use cases. It introduces a BarData wrapper that tracks data mutations via version numbers, enabling automatic cache invalidation, and adds a materialize() method to break closure chains that cause memory leaks in complex indicator calculations.
Key Changes
- BarData wrapper: Automatically increments version on mutations (
push(),pop(),set(),updateLast(),setAll()), enabling Series to detect stale caches - Automatic cache invalidation: Series checks BarData version before returning cached values, recomputing only when data has changed
- materialize() method: Breaks closure chains by eagerly computing values and creating a fresh Series without parent references, critical for complex multi-operation chains
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/oakscriptjs/src/runtime/series.ts | Adds BarData class with version tracking, updates Series constructor to accept Bar[] or BarData, implements version-based cache invalidation in toArray(), adds materialize() method |
| packages/oakscriptjs/src/index.ts | Exports BarData class alongside Series for public API |
| packages/oakscriptjs/tests/runtime/series.test.ts | Comprehensive test coverage for BarData mutations, cache invalidation scenarios, materialize() functionality, backward compatibility, and integration tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Create a copy of values to ensure no shared references | ||
| const valuesCopy = [...values]; | ||
| return Series.fromArray(this.dataSource, valuesCopy); |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The array spread operation [...values] creates an unnecessary copy. Since toArray() already returns a fresh computation when cache is invalidated, and the cache is not shared between Series instances, the copy is redundant. This adds O(N) time and space overhead on every materialize() call.
Consider removing the spread and directly using the values:
materialize(): Series {
const values = this.toArray();
return Series.fromArray(this.dataSource, values);
}The values array is already safe to use directly because:
toArray()returns a fresh array when recomputingSeries.fromArray()creates a new Series that captures the array by reference- The materialized Series won't modify the array
| // Create a copy of values to ensure no shared references | |
| const valuesCopy = [...values]; | |
| return Series.fromArray(this.dataSource, valuesCopy); | |
| return Series.fromArray(this.dataSource, values); |
| get bars(): Bar[] { | ||
| return this._bars; |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bars getter exposes the internal array by reference, allowing external code to bypass version tracking. While this is documented in the JSDoc (lines 52-54), it creates a significant footgun that could lead to subtle cache invalidation bugs.
Consider strengthening the API design:
- Return a readonly array type:
get bars(): readonly Bar[]to make mutations a type error - Or provide an internal
_getBarsUnsafe()method for Series to use, while makingbarsreturn a copy
The current documentation warns users but doesn't prevent the misuse at compile-time.
| get bars(): Bar[] { | |
| return this._bars; | |
| get bars(): readonly Bar[] { | |
| // Return a shallow copy to prevent external mutation | |
| return this._bars.slice(); |
| // Create a copy of values to ensure no shared references | ||
| const valuesCopy = [...values]; | ||
| return Series.fromArray(this.dataSource, valuesCopy); |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The variable name valuesCopy is misleading because it suggests the copy is necessary, but based on the implementation, the copy appears redundant. If the copy is actually needed for safety (e.g., to prevent the materialized Series from being affected by cache clearing in the source Series), this should be documented.
Consider either:
- Removing the copy if it's truly unnecessary (see related comment on lines 514-515)
- Or renaming to
isolatedValueswith a comment explaining why isolation is needed
| // Create a copy of values to ensure no shared references | |
| const valuesCopy = [...values]; | |
| return Series.fromArray(this.dataSource, valuesCopy); | |
| // Create a copy to isolate the materialized Series from future cache invalidation or mutation in the source Series. | |
| const isolatedValues = [...values]; | |
| return Series.fromArray(this.dataSource, isolatedValues); |
| toArray(): number[] { | ||
| if (this.cached !== null) { | ||
| // Check if cache is valid by comparing versions | ||
| if (this.cached !== null && this.cachedVersion === this.dataSource.version) { | ||
| return this.cached; | ||
| } | ||
|
|
||
| this.cached = this.data.map((bar, i) => this.extractor(bar, i, this.data)); | ||
| // Recompute and update cache | ||
| const bars = this.dataSource.bars; | ||
| this.cached = bars.map((bar, i) => this.extractor(bar, i, bars)); | ||
| this.cachedVersion = this.dataSource.version; | ||
| return this.cached; | ||
| } |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The toArray() method creates a new array on every cache miss, but there's no mechanism to limit memory growth if the BarData keeps growing (e.g., in a long-running streaming application). Each Series holds a full cached copy of all values.
For production streaming scenarios with unbounded bar data, consider adding:
- A maximum cache size or TTL mechanism
- Documentation about memory implications for long-running streams
- A
clearCache()method users can call periodically
Example scenario: A streaming app running for days/weeks could accumulate millions of bars, with each derived Series holding a full array copy in cache.
| * Get a bar at a specific index | ||
| * @param index - Bar index | ||
| * @returns Bar at that index, or undefined | ||
| */ | ||
| at(index: number): Bar | undefined { | ||
| return this._bars[index]; |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The at() method doesn't validate the index parameter, which could lead to unexpected behavior with negative indices due to JavaScript's array indexing.
For example:
barData.at(-1) // Returns the last element (standard JS behavior)
barData.at(-100) // Returns undefinedWhile this matches JavaScript's native Array.at() behavior, it may be surprising for users who expect array-style bounds checking. Consider:
- Documenting that negative indices are supported (following Array.at() convention)
- Or explicitly rejecting negative indices if that's not the intended behavior
The current implementation is silent about this edge case.
| * Get a bar at a specific index | |
| * @param index - Bar index | |
| * @returns Bar at that index, or undefined | |
| */ | |
| at(index: number): Bar | undefined { | |
| return this._bars[index]; | |
| * Get a bar at a specific index (supports negative indices, like Array.at) | |
| * @param index - Bar index (negative values count from the end) | |
| * @returns Bar at that index, or undefined | |
| */ | |
| at(index: number): Bar | undefined { | |
| return this._bars.at(index); |
| ]; | ||
|
|
||
| const close = Series.fromBars(bars, 'close'); | ||
| const open = Series.fromBars(bars, 'open'); |
Copilot
AI
Dec 5, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused variable open.
| const open = Series.fromBars(bars, 'open'); |
The Series class had no automatic cache invalidation when underlying data mutated, and nested operations created O(N) closure chains that retained all intermediate Series objects in memory.
Changes
BarData wrapper for automatic cache invalidation
Bar[]with version tracking that increments on mutations (push(),pop(),set(),updateLast(),setAll())toArray()and recomputes only when data changedmaterialize() method to break closure chains
a.add(b).mul(c).materialize().div(d)frees references to a, b, cBackward compatibility
Bar[]arrays automatically wrapped in BarData internallyfromBars(),constant(),fromArray()) accept both typesExample
Performance impact: O(1) version check overhead on cache hits, otherwise identical.
Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.