Thread safety per request only#70
Closed
cavusmustafa wants to merge 321 commits intoravi9:dev_backend_openvinofrom
Closed
Thread safety per request only#70cavusmustafa wants to merge 321 commits intoravi9:dev_backend_openvinofrom
cavusmustafa wants to merge 321 commits intoravi9:dev_backend_openvinofrom
Conversation
Co-authored-by: Yamini Nimmagadda <yamini.nimmagadda@intel.com>
…d node retrieval inside guarded block to prevent missing-key access
…embedding Fix VIEW op, which slice the input node
…miss Fix missing issue key handling
Fix for stateful execution bug in llama-bench
cavusmustafa
requested review from
Copilot and
wine99
and removed request for
wine99
There was a problem hiding this comment.
Pull request overview
This PR adjusts the OpenVINO backend’s decoder caching and locking strategy to reduce cross-request contention by moving from a single global mutex to per-decoder (per-cache-entry) mutexes, and removes the global lock previously used during weight-node creation.
Changes:
- Introduces
decoder_runtime_ctxto pair each cached decoder with its own mutex and updatesdecoder_cacheto store this context. - Updates dynamic/static graph compute paths to lock per-decoder mutexes instead of a single runtime mutex.
- Removes the static
weights_mutexfromGgmlOvDecoder::create_weight_nodes().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
ggml/src/ggml-openvino/utils.h |
Changes decoder cache value type to include per-entry mutex + decoder pointer. |
ggml/src/ggml-openvino/utils.cpp |
Reworks cache access/locking to use per-entry mutexes and updates cache writes accordingly. |
ggml/src/ggml-openvino/ggml-decoder.cpp |
Removes the global mutex guarding weight-node creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+43
to
+47
| struct decoder_runtime_ctx { | ||
| decoder_runtime_ctx(std::shared_ptr<std::mutex> mutex) : | ||
| mutex(mutex) {} | ||
| std::shared_ptr<std::mutex> mutex; | ||
| std::shared_ptr<GgmlOvDecoder> ptr; |
Comment on lines
+109
to
124
| std::shared_ptr<std::mutex> mutex; | ||
|
|
||
| auto it = r_ctx->decoder_cache.find(key); | ||
|
|
||
| cache_hit = it != r_ctx->decoder_cache.end(); | ||
| ModelParams old_m_params; | ||
| if (cache_hit) { | ||
| ggml_decoder = it->second; | ||
| mutex = it->second->mutex; | ||
| std::lock_guard<std::mutex> lock(*(mutex)); | ||
| ggml_decoder = it->second->ptr; | ||
| old_m_params = ggml_decoder->get_model_params(); | ||
| cache_hit = old_m_params.can_reuse_dynamically(m_params); | ||
| } else { | ||
| mutex = std::make_shared<std::mutex>(); | ||
| r_ctx->decoder_cache[key] = std::make_shared<decoder_runtime_ctx>(mutex); | ||
| } |
Comment on lines
115
to
126
| if (cache_hit) { | ||
| ggml_decoder = it->second; | ||
| mutex = it->second->mutex; | ||
| std::lock_guard<std::mutex> lock(*(mutex)); | ||
| ggml_decoder = it->second->ptr; | ||
| old_m_params = ggml_decoder->get_model_params(); | ||
| cache_hit = old_m_params.can_reuse_dynamically(m_params); | ||
| } else { | ||
| mutex = std::make_shared<std::mutex>(); | ||
| r_ctx->decoder_cache[key] = std::make_shared<decoder_runtime_ctx>(mutex); | ||
| } | ||
| std::lock_guard<std::mutex> lock(*(mutex)); | ||
|
|
Comment on lines
322
to
+331
| if (cache_hit) { | ||
| ggml_decoder = it->second; | ||
| ggml_decoder = it->second->ptr; | ||
| mutex = it->second->mutex; | ||
| old_m_params = ggml_decoder->get_model_params(); | ||
| cache_hit = old_m_params.can_reuse_statically(m_params); | ||
| } else { | ||
| mutex = std::make_shared<std::mutex>(); | ||
| r_ctx->decoder_cache[key] = std::make_shared<decoder_runtime_ctx>(mutex); | ||
| } | ||
| std::lock_guard<std::mutex> lock(*(mutex)); |
| } | ||
| } | ||
| } | ||
| infer_request->wait(); |
Comment on lines
574
to
576
| std::map<std::string, std::shared_ptr<ov::Node>> GgmlOvDecoder::create_weight_nodes(ggml_cgraph * cgraph, bool naive) { | ||
| static std::mutex weights_mutex; | ||
| std::lock_guard<std::mutex> lock(weights_mutex); | ||
|
|
||
| std::map<std::string, std::shared_ptr<ov::Node>> model_weights; | ||
| auto * nodes = cgraph->nodes; |
wine99
force-pushed
the
dev_backend_openvino
branch
from
996b739 to
b6c83aa
Compare
Collaborator
|
LGTM. Let's wait for the internal CI is setup and see the CI result |
Collaborator
Author
|
It seems like one thread safety test is failing. I will check and update. |
Collaborator
Author
|
Too many conflicts to resolve so it seems to be safer to create a clean PR. I will close this one and check if CI passes for the new one: #73 |
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
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.
create_weight_nodesfunction. Thread safety test seems to be passing fine but need to see results for full CI test.