Optimize ML prediction hot path: circular buffer and reduce allocations#22042
Optimize ML prediction hot path: circular buffer and reduce allocations#22042stelfrag wants to merge 16 commits intonetdata:masterfrom
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 6 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant API as ML API (ml.cc)
participant Dim as ml_dimension_t (State)
participant Feat as ML Features (Preprocessing)
participant KM as ML KMeans (Model)
participant Worker as ML Worker (Training Cache)
alt Prediction Flow (Hot Path)
API->>Dim: NEW: Update circular buffer at cns_head
Dim->>Dim: Increment cns_head (wrap around)
API->>API: Reconstruct contiguous cns array from ring buffer
API->>Feat: NEW: ml_features_preprocess_predict()
Feat->>Feat: ml_features_diff()
Feat->>Feat: ml_features_smooth()
Feat-->>Dim: NEW: Fill inline DSample (dim->feature)
Note right of Feat: Skips lag step & heap allocations
loop for each KMeans context
API->>KM: ml_kmeans_anomaly_score(dim->feature)
KM-->>API: anomaly_score
end
else Training Flow
API->>Worker: Access training_samples scratchpad
API->>Feat: CHANGED: ml_features_preprocess()
Note right of API: Passes &worker->training_samples (Pointer)
Feat->>Feat: ml_features_diff()
Feat->>Feat: ml_features_smooth()
Feat->>Feat: CHANGED: ml_features_lag()
Feat-->>Worker: Populate training_samples vector
API->>KM: CHANGED: ml_kmeans_train()
Note right of KM: Dereferences preprocessed_features pointer
KM->>Worker: Read training samples
KM-->>Dim: Update KMeans model
end
opt Missing Data
API->>Dim: NEW: Reset cns_head = 0
API->>Dim: Clear cns buffer
end
…ar buffer, optimize feature preprocessing, and improve memory management.
There was a problem hiding this comment.
Pull request overview
Optimizes the ML prediction hot path by switching from std::rotate to a circular buffer, inlining the per-dimension feature sample, and adding a prediction-specific preprocessing function to avoid per-tick allocations and training-only work.
Changes:
- Replace rotating sample history with a ring buffer (
cns_head) and reconstruct contiguous arrays for preprocessing. - Add
ml_features_preprocess_predict()to build a singleDSamplefor prediction without creating astd::vector<DSample>. - Change
ml_features_t.preprocessed_featuresfrom a reference to a nullable pointer so prediction can passnullptr.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ml/ml_public.cc | Resets cns_head on host stop to keep the ring buffer consistent. |
| src/ml/ml_kmeans.cc | Updates k-means training to dereference preprocessed_features via pointer. |
| src/ml/ml_features.h | Makes preprocessed_features nullable and adds ml_features_preprocess_predict(). |
| src/ml/ml_features.cc | Implements prediction-only preprocessing and updates lag preprocessing to use the pointer. |
| src/ml/ml_dimension.h | Adds cns_head and inlines feature as a single DSample. |
| src/ml/ml.cc | Switches prediction path to ring buffer + prediction preprocess + inline DSample. |
| src/ml/ml-unittest.cc | Updates unit tests for the preprocessed_features pointer API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add null checks for `preprocessed_features` in preprocessing and k-means training. - Avoid crashes in `ml_kmeans_train` when features are uninitialized. - Fix logical error in value comparison during circular buffer update.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/ml/ml.cc">
<violation number="1" location="src/ml/ml.cc:780">
P2: Compare against the most recent sample, not the oldest element at cns_head. Using cns_head here misclassifies whether the incoming value matches the latest value.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Resolve logical error in determining the newest index for value comparison during circular buffer updates.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…res` in ML preprocessing
- Compare incoming value against the newest sample instead of the oldest. - Prevent misclassification of constant vs variable dimensions during training and statistics reporting. - Ensure reliable detection of transitions in changing series.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/ml/ml.cc:1
- The stack buffers
src_cns/dst_cnsare sized to 128 elements, but the assert still usesn * (Cfg.lag_n + 1) <= 128. After switching prediction toml_features_preprocess_predict()(which does not perform lag expansion), this check can now falsely fail for validn(e.g.,n=64, lag_n=2), preventing prediction even though the buffers would be sufficient. Update the assert to validate the actual buffers used (e.g.,n <= 128) or size the arrays based on the asserted expression.
// SPDX-License-Identifier: GPL-3.0-or-later
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…d improve memory safety - Remove redundant pointer-based management of preprocessed features, replacing it with `std::vector` references for cleaner and safer memory handling. - Update `ml_features_preprocess` and `ml_kmeans_train` to use direct vector references. - Adjust all related test cases and logic to align with the refactored interfaces. - Add unit test to validate correct comparison of the newest sample in `ml_dimension_predict`.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/ml/ml.cc:803
- The static-buffer guard uses
n * (Cfg.lag_n + 1) <= 128, but this prediction path only storesncalculated_numbers intosrc_cns/dst_cnsandml_features_preprocess_predict()reads at mostlag_n + 1values. Using the product makes the constraint harder to reason about and could become unnecessarily restrictive if feature window sizing changes. Prefer asserting the actual requirements (e.g.,n <= 128andlag_n + 1 <= n) to match the current memory bounds.
// Create the sample
assert((n * (Cfg.lag_n + 1) <= 128) &&
"Static buffers too small to perform prediction. "
"This should not be possible with the default clamping of feature extraction options");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…in ML preprocessing - Introduced `ml_effective_smooth_n` to handle `smooth_n=0` as equivalent to `smooth_n=1`. - Updated smoothing logic for consistency across preprocessing and prediction. - Added comprehensive unit test `test_features_zero_smooth_matches_one` to validate feature consistency for `smooth_n=0` and `smooth_n=1`.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Treat `smooth_n=0` as equivalent to a smoothing window of 1 across all paths. - Update signature of `ml_features_preprocess_predict` to use `DSample` references instead of pointers for improved safety. - Adjust tests, comments, and logic to reflect updated `smooth_n=0` behavior and interface changes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Ensure buffer clears and resets when smoothing window changes to maintain state consistency. - Replace `assert` with `fatal_assert` for static buffer size validation during prediction.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add `cns_size` optimization to reduce redundant size checks. - Clarify logic for detecting effective window changes and resetting buffer state. - Improve comments for consistency in ring buffer handling.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/ml/ml.cc">
<violation number="1" location="src/ml/ml.cc:791">
P2: Rebuild the ring buffer whenever the effective window changes, even if the buffer is currently smaller than the new size. The new `cns_size >= n` guard skips the reset on window growth, leaving `cns_head` tied to the old modulus and corrupting sample order once the buffer refills.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/ml/ml-unittest.cc:489
- The new
ml_features_preprocess_predict()behavior is only directly exercised intest_features_zero_smooth_matches_one()(and only for a single small input/parameter set).test_preprocess_predict_equivalence()still validates a “manual extraction” path rather than callingml_features_preprocess_predict()itself. To better lock in correctness, update that test to callml_features_preprocess_predict()across the existing parameter sets/patterns and compare itsDSampleoutput topf[0]fromml_features_preprocess().
static void test_preprocess_predict_equivalence()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tion paths - Replace manual feature extraction logic in unit tests with `ml_features_preprocess_predict`. - Add input validation via `ml_validate_features_input` to both training and prediction paths. - Ensure consistent buffer validation and minimum sample checks across preprocessing functions.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
thiagoftsm
left a comment
thiagoftsm
left a comment
There was a problem hiding this comment.
PR is working as expected. LGTM!
Summary
Summary by cubic
Speeds up the ML prediction hot path with a ring buffer, unified prediction preprocessing, and simpler APIs to reduce allocations. Adds strict input validation and normalizes smoothing to keep state consistent and accurate.
Refactors
dim->cns; linearize fromcns_head; rebuild (clear and reset head) when the effective smoothing window changes or wrapped state is incompatible; usefatal_assertfor static buffer checks.ml_features_preprocess_predictthat fills a singleDSample; training usesml_features_preprocess(features, out)andml_kmeans_train(kmeans, out); store oneDSampleonml_dimension_t; compute per-dimension windows viaml_dimension_smoothing_window.Bug Fixes
smooth_n=0to behave likesmooth_n=1across preprocessing and prediction; unit test ensures consistency.cns/cns_headon missing values and host stop to avoid stale indexing.-std=gnu++14.Written for commit 69045e4. Summary will update on new commits.