Repository navigation
Conversation
…preserving default behavior This PR exposes ONNX Runtime memory allocation settings so users can control: - `enable_mem_pattern` - `enable_cpu_mem_arena` The main motivation is to allow bindings and downstream users to disable ONNX Runtime memory arena behavior when needed, while keeping the current default behavior unchanged.
dosubot
Bot
added
the
size:L
negbie
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds two ONNX Runtime tuning flags— Changes
Sequence Diagram(s)sequenceDiagram
participant C as "C API / Caller"
participant Cpp as "C++ API / Wrappers"
participant Prov as "ProviderConfig"
participant Sess as "SessionOptions"
participant ORT as "ONNX Runtime"
rect rgba(200,200,255,0.5)
C->>Cpp: pass model/config (may include int flags)
end
rect rgba(200,255,200,0.5)
Cpp->>Prov: construct ProviderConfig(provider, enable_cpu_mem_arena, enable_mem_pattern)
end
rect rgba(255,200,200,0.5)
Prov->>Sess: GetSessionOptionsImpl(num_threads, provider, &Prov)
Sess->>ORT: if enable_mem_pattern == false -> DisableMemPattern()
Sess->>ORT: if enable_cpu_mem_arena == false -> DisableCpuMemArena()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for configuring ONNX Runtime's CPU memory arena and memory pattern optimization across various components in the C and C++ APIs. Reviewers identified a logic error in the C API where the default value prevents users from disabling these features, a compilation error due to missing fields in the C++ OfflineModelConfig struct, and an issue where these settings are not correctly propagated to components that do not use ProviderConfig.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sherpa-onnx/c-api/c-api.cc`:
- Around line 114-117: The code currently uses SHERPA_ONNX_OR(config->..., 1)
which coerces an explicit 0 to 1 so callers cannot disable flags; replace those
OR uses with a presence check that preserves explicit 0, e.g. use
SHERPA_ONNX_HAS(config->model_config.enable_cpu_mem_arena) ?
config->model_config.enable_cpu_mem_arena : 1 (and similarly for
enable_mem_pattern), and apply the same pattern to the other affected builders
(the places setting
recognizer_config.model_config.provider_config.enable_cpu_mem_arena,
enable_mem_pattern and the five other analogous flag assignments referenced in
the comment so an explicit 0 from the C API is honored).
In `@sherpa-onnx/c-api/c-api.h`:
- Around line 245-248: You added new fields into public C structs (e.g.,
SherpaOnnxOnlineModelConfig, SherpaOnnxOfflineModelConfig,
SherpaOnnxVadModelConfig, etc.), which breaks ABI; instead introduce versioned
structs (e.g., SherpaOnnxOnlineModelConfigV2, SherpaOnnxOfflineModelConfigV2,
SherpaOnnxVadModelConfigV2, etc.) that include the two new int32_t fields, leave
the original structs unchanged, and add new factory/create functions (e.g.,
SherpaOnnxCreateOnlineModelConfigV2 or SherpaOnnxOnlineModelConfig_CreateV2) and
overloaded/new model-creation APIs that accept the V2 types; internally convert
or map V2 -> internal representation used by existing code paths so runtime
behavior is unchanged for older binaries while new binaries can opt into V2
fields.
In `@sherpa-onnx/c-api/cxx-api.cc`:
- Around line 276-278: The C++ wrapper struct OfflineModelConfig is missing the
fields that the C API struct SherpaOnnxOfflineModelConfig exposes; add bool
enable_cpu_mem_arena and bool enable_mem_pattern to OfflineModelConfig in
cxx-api.h (with sensible defaults), then update the conversion helpers used in
cxx-api.cc (the functions that map between SherpaOnnxOfflineModelConfig and
OfflineModelConfig at the sites referencing these fields) to read/write these
two fields so the assignments at lines ~106–108, ~276–278 and ~701–703 compile
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f846d7b0-1754-4f90-b132-5a4b1f4ec24a
📒 Files selected for processing (7)
sherpa-onnx/c-api/c-api.ccsherpa-onnx/c-api/c-api.hsherpa-onnx/c-api/cxx-api.ccsherpa-onnx/c-api/cxx-api.hsherpa-onnx/csrc/provider-config.ccsherpa-onnx/csrc/provider-config.hsherpa-onnx/csrc/session.cc
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sherpa-onnx/c-api/c-api.cc`:
- Around line 72-76: The resolver SherpaOnnxResolveOptionalBool currently treats
any negative v as "default" (v < 0) which contradicts its tri-state comment;
change the check to only treat v == -1 as the default case so that other
negative values are evaluated as non-zero/true or zero/false accordingly (update
the conditional in SherpaOnnxResolveOptionalBool to use v == -1 and keep the
fallback as (v != 0) with default_value returned when v == -1).
- Around line 72-76: The three offline C API builder functions
GetOfflineTtsConfig, GetOfflinePunctuationConfig, and
GetOfflineSpeechDenoiserConfig currently ignore the C struct fields
enable_cpu_mem_arena and enable_mem_pattern; update each builder to read those
fields from the corresponding C structs (SherpaOnnxOfflineTtsConfig,
SherpaOnnxOfflinePunctuationConfig, SherpaOnnxOfflineSpeechDenoiserConfig) and
assign them into the created C++ config via SherpaOnnxResolveOptionalBool (using
the same default semantics as the other offline builders). Follow the pattern
used in the existing builders (e.g.,
GetOfflineRecognizerConfig/GetOfflineVadConfig): call
SherpaOnnxResolveOptionalBool(arg->enable_cpu_mem_arena, default_value) and
SherpaOnnxResolveOptionalBool(arg->enable_mem_pattern, default_value) and set
the resulting booleans on the returned config object.
In `@sherpa-onnx/csrc/vad-model-config.h`:
- Around line 30-33: The VadModelConfig constructor currently requires the new
boolean flags enable_cpu_mem_arena and enable_mem_pattern which breaks existing
positional calls; update the constructor signature for VadModelConfig(const
SileroVadModelConfig&, const TenVadModelConfig&, int32_t sample_rate, int32_t
num_threads, const std::string& provider, bool enable_cpu_mem_arena = true, bool
enable_mem_pattern = true, bool debug) so both flags default to true, preserving
source compatibility and old behavior for existing call sites that do not pass
these arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5974c839-1cbe-4e47-86da-a71d2d40f273
📒 Files selected for processing (15)
sherpa-onnx/c-api/c-api.ccsherpa-onnx/c-api/c-api.hsherpa-onnx/c-api/cxx-api.hsherpa-onnx/csrc/audio-tagging-model-config.hsherpa-onnx/csrc/offline-model-config.hsherpa-onnx/csrc/offline-punctuation-model-config.hsherpa-onnx/csrc/offline-source-separation-model-config.hsherpa-onnx/csrc/offline-speaker-segmentation-model-config.hsherpa-onnx/csrc/offline-speech-denoiser-model-config.hsherpa-onnx/csrc/offline-tts-model-config.hsherpa-onnx/csrc/online-punctuation-model-config.hsherpa-onnx/csrc/session.hsherpa-onnx/csrc/speaker-embedding-extractor.hsherpa-onnx/csrc/spoken-language-identification.hsherpa-onnx/csrc/vad-model-config.h
✅ Files skipped from review due to trivial changes (1)
- sherpa-onnx/c-api/c-api.h
🚧 Files skipped from review as they are similar to previous changes (1)
- sherpa-onnx/c-api/cxx-api.h
…uctor source compatibility
There was a problem hiding this comment.
♻️ Duplicate comments (1)
sherpa-onnx/c-api/c-api.cc (1)
72-76:⚠️ Potential issue | 🟡 MinorResolver semantics still don’t match the documented tri-state contract.
On Line 75,
v < 0treats all negative values as “default”, but the comment on Line 72 documents only-1as default. This keeps behavior inconsistent for values like-2.🔧 Proposed fix
static inline bool SherpaOnnxResolveOptionalBool(int32_t v, bool default_value) { - return v < 0 ? default_value : (v != 0); + return v == -1 ? default_value : (v != 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sherpa-onnx/c-api/c-api.cc` around lines 72 - 76, The resolver treats any negative int as "default" but the contract says only -1 should be default; update SherpaOnnxResolveOptionalBool to check for v == -1 instead of v < 0 so only -1 returns default_value, e.g. change the condition in SherpaOnnxResolveOptionalBool to "v == -1 ? default_value : (v != 0)"; keep the rest of the logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@sherpa-onnx/c-api/c-api.cc`:
- Around line 72-76: The resolver treats any negative int as "default" but the
contract says only -1 should be default; update SherpaOnnxResolveOptionalBool to
check for v == -1 instead of v < 0 so only -1 returns default_value, e.g. change
the condition in SherpaOnnxResolveOptionalBool to "v == -1 ? default_value : (v
!= 0)"; keep the rest of the logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f26d28ca-d313-41fa-a4c6-bdb9c3816831
📒 Files selected for processing (2)
sherpa-onnx/c-api/c-api.ccsherpa-onnx/csrc/vad-model-config.h
🚧 Files skipped from review as they are similar to previous changes (1)
- sherpa-onnx/csrc/vad-model-config.h
Collaborator
|
Thank you for your contribution! Instead of introducing more fields, please use a config file, which is already supported. What you need to do is to change only See also sherpa-onnx/sherpa-onnx/csrc/session.cc Lines 165 to 172 in 48a3e36 You use it like and |
Contributor
Author
|
Ok thanks. I will open a new pr. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR exposes ONNX Runtime memory allocation settings so users can control:
enable_mem_patternenable_cpu_mem_arenaThe main motivation is to allow bindings and downstream users to disable ONNX Runtime memory arena behavior when needed, while keeping the current default behavior unchanged.
Summary by CodeRabbit