[Not for merge]: Support mmap#3484
Conversation
![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 implements memory-mapping for model loading in OfflineQwen3ASRModel and OfflineSenseVoiceModel on non-Windows systems, enhancing performance. It also updates model configuration descriptions to include support for the .ort format. Review feedback suggests centralizing the mmap utility functions to a shared file to eliminate code duplication, adopting a consistent MappedModel structure across different model implementations, using the standard int type for errno, and refactoring repetitive initialization methods to reduce boilerplate.
| void MapModel(const std::string &filename, const char *name, | ||
| MappedModel *mapped_model) { | ||
| int fd = open(filename.c_str(), O_RDONLY); | ||
| if (fd == -1) { | ||
| SHERPA_ONNX_LOGE("Failed to open %s '%s': %s", name, filename.c_str(), | ||
| std::strerror(errno)); | ||
| SHERPA_ONNX_EXIT(-1); | ||
| } | ||
|
|
||
| struct stat st; | ||
| if (fstat(fd, &st) == -1) { | ||
| SHERPA_ONNX_LOGE("Failed to stat %s '%s': %s", name, filename.c_str(), | ||
| std::strerror(errno)); | ||
| close(fd); | ||
| SHERPA_ONNX_EXIT(-1); | ||
| } | ||
|
|
||
| if (st.st_size <= 0) { | ||
| SHERPA_ONNX_LOGE("%s '%s' has invalid size: %lld", name, filename.c_str(), | ||
| static_cast<long long>(st.st_size)); | ||
| close(fd); | ||
| SHERPA_ONNX_EXIT(-1); | ||
| } | ||
|
|
||
| void *data = mmap(nullptr, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0); | ||
| int32_t saved_errno = errno; | ||
| close(fd); | ||
|
|
||
| if (data == MAP_FAILED) { | ||
| SHERPA_ONNX_LOGE("Failed to mmap %s '%s': %s", name, filename.c_str(), | ||
| std::strerror(saved_errno)); | ||
| SHERPA_ONNX_EXIT(-1); | ||
| } | ||
|
|
||
| mapped_model->data = data; | ||
| mapped_model->size = static_cast<size_t>(st.st_size); | ||
| } | ||
|
|
||
| void UnmapModel(MappedModel *mapped_model) { | ||
| if (mapped_model->data) { | ||
| munmap(mapped_model->data, mapped_model->size); | ||
| mapped_model->data = nullptr; | ||
| mapped_model->size = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for memory-mapping models (MapModel and UnmapModel) is duplicated here and in offline-sense-voice-model.cc. Following the general rules for this repository, these utility functions should be moved to a common utility file (e.g., sherpa-onnx/csrc/mmap-utils.h) to improve maintainability and reduce code duplication.
References
- Move duplicated utility functions, such as
Trim, to a common utility file (e.g.,text-utils.handtext-utils.cc) for reuse across the codebase.
| void *mapped_model_data_ = nullptr; | ||
| size_t mapped_model_size_ = 0; |
There was a problem hiding this comment.
For consistency with the implementation in offline-qwen3-asr-model.cc, consider using a MappedModel struct to group the mapped data pointer and its size. This would also facilitate refactoring the mmap logic into a shared utility.
References
- Move duplicated utility functions, such as
Trim, to a common utility file (e.g.,text-utils.handtext-utils.cc) for reuse across the codebase.
| } | ||
|
|
||
| void *data = mmap(nullptr, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0); | ||
| int32_t saved_errno = errno; |
There was a problem hiding this comment.
errno is defined as an int. While int32_t is often the same size on modern platforms, it is more idiomatic to use int when saving errno.
int saved_errno = errno;| void InitConvFrontendFromFile(const std::string &filename) { | ||
| #if defined(_WIN32) | ||
| conv_sess_ = std::make_unique<Ort::Session>(env_, | ||
| SHERPA_ONNX_TO_ORT_PATH(filename), | ||
| sess_opts_conv_); | ||
| InitConvFrontend(nullptr, 0); | ||
| #else | ||
| MapModel(filename, "qwen3_asr.conv_frontend", &conv_model_); | ||
| InitConvFrontend(conv_model_.data, conv_model_.size); | ||
| #endif | ||
| } | ||
|
|
||
| void InitEncoderFromFile(const std::string &filename) { | ||
| #if defined(_WIN32) | ||
| encoder_sess_ = std::make_unique<Ort::Session>( | ||
| env_, SHERPA_ONNX_TO_ORT_PATH(filename), sess_opts_encoder_); | ||
| InitEncoder(nullptr, 0); | ||
| #else | ||
| MapModel(filename, "qwen3_asr.encoder", &encoder_model_); | ||
| InitEncoder(encoder_model_.data, encoder_model_.size); | ||
| #endif | ||
| } | ||
|
|
||
| void InitDecoderFromFile(const std::string &filename) { | ||
| #if defined(_WIN32) | ||
| decoder_sess_ = std::make_unique<Ort::Session>( | ||
| env_, SHERPA_ONNX_TO_ORT_PATH(filename), sess_opts_decoder_); | ||
| InitDecoder(nullptr, 0); | ||
| #else | ||
| MapModel(filename, "qwen3_asr.decoder", &decoder_model_); | ||
| InitDecoder(decoder_model_.data, decoder_model_.size); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
The methods InitConvFrontendFromFile, InitEncoderFromFile, and InitDecoderFromFile follow an identical pattern. Consider refactoring this into a single helper method that takes the filename, model name, session pointer, session options, and the specific initialization function as arguments to reduce boilerplate code.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
1 participant
Fixes #3226
I evaluated the Qwen3 ASR and SenseVoice int8 models. My tests show that enabling mmap does not yield any noticeable improvement in loading latency. Additionally, converting the ONNX models to .ort format also failed to reduce the loading time.