[None][fix] Add mutex to avoid potentially concurrent modifications to std::unordered_map#16439
[None][fix] Add mutex to avoid potentially concurrent modifications to std::unordered_map#16439yihwang-nv wants to merge 3 commits into
std::unordered_map#16439Conversation
|
/bot run |
|
PR_Github #59474 [ run ] triggered by Bot. Commit: |
WalkthroughThe PR replaces the removed custom hash functor with internal ChangesHashing and cache infrastructure
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/tensorrt_llm/thop/attentionOp.cpp (1)
1309-1313: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPopulate the cache on a miss. The locked branch only assigns when
op_cache.find(cache_key) != op_cache.end(), so new entries are never inserted and this cache never hits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tensorrt_llm/thop/attentionOp.cpp` around lines 1309 - 1313, Update the attention operation cache logic around op_cache.find so a cache miss creates or obtains the attention operation and inserts it under cache_key, while preserving assignment from it->second on hits. Ensure the newly populated entry is available for subsequent lookups.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tensorrt_llm/common/opUtils.h`:
- Around line 110-122: Rename the helper variable templates __is_tuple_like and
__is_iterable to non-reserved identifiers, and update every reference to those
traits consistently. Preserve their existing tuple and iterable detection
behavior.
- Around line 135-169: Extract the duplicated hash-combine literal 0x9e3779b9
into a shared named constant using the k-prefixed camelCase convention, then
replace both uses in hash_helper’s iterable specialization and tuple
specialization with that constant.
In `@cpp/tensorrt_llm/thop/attentionOp.cpp`:
- Line 1308: Rename the newly introduced mutex from op_cache_mutex to
opCacheMutex in the surrounding function, and update its use at the
corresponding lock site to match the lower camel case naming convention.
- Around line 1320-1323: The cache insertion condition in the op cache update
block is inverted, preventing missing entries from being stored. Update the
logic around op_cache_mutex to insert op only when cache_key is absent, while
preserving any value already populated by another thread.
---
Outside diff comments:
In `@cpp/tensorrt_llm/thop/attentionOp.cpp`:
- Around line 1309-1313: Update the attention operation cache logic around
op_cache.find so a cache miss creates or obtains the attention operation and
inserts it under cache_key, while preserving assignment from it->second on hits.
Ensure the newly populated entry is available for subsequent lookups.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fc101a8a-d31e-4b13-a090-4f637a61e683
📒 Files selected for processing (2)
cpp/tensorrt_llm/common/opUtils.hcpp/tensorrt_llm/thop/attentionOp.cpp
| template <class Tuple> | ||
| inline constexpr bool __is_tuple_like = false; | ||
|
|
||
| template <class... Args> | ||
| inline constexpr bool __is_tuple_like<std::tuple<Args...>> = true; | ||
|
|
||
| template <class T, class Enable = void> | ||
| inline constexpr bool __is_iterable = false; | ||
|
|
||
| template <class T> | ||
| inline constexpr bool | ||
| __is_iterable<T, std::void_t<decltype(std::begin(std::declval<T&>())), decltype(std::end(std::declval<T&>()))>> | ||
| = true; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant portion of the header.
sed -n '100,190p' cpp/tensorrt_llm/common/opUtils.h | cat -n
# Find all uses of the double-underscore traits in the repo.
rg -n "__is_tuple_like|__is_iterable" cpp/tensorrt_llm
# Check the repository guidance file if present.
fd -a "CODING_GUIDELINES.md" .Repository: NVIDIA/TensorRT-LLM
Length of output: 3760
Rename __is_tuple_like and __is_iterable. Double-underscore identifiers are reserved to the implementation, even inside this anonymous namespace.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/tensorrt_llm/common/opUtils.h` around lines 110 - 122, Rename the helper
variable templates __is_tuple_like and __is_iterable to non-reserved
identifiers, and update every reference to those traits consistently. Preserve
their existing tuple and iterable detection behavior.
|
PR_Github #59474 [ run ] completed with state
|
tongyuantongyu
changed the title
std::unordered_mapstd::unordered_map
|
/bot run |
|
PR_Github #60265 [ run ] triggered by Bot. Commit: |
|
PR_Github #60265 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60285 [ run ] triggered by Bot. Commit: |
|
PR_Github #60285 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #60324 [ run ] triggered by Bot. Commit: |
|
PR_Github #60324 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #60400 [ run ] triggered by Bot. Commit: |
|
PR_Github #60400 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #60599 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #60666 [ run ] triggered by Bot. Commit: |
|
PR_Github #60599 [ run ] completed with state |
|
PR_Github #60666 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #60859 [ run ] triggered by Bot. Commit: |
|
PR_Github #60859 [ run ] completed with state |
| size_t operator()(T const& v) const | ||
| { | ||
| return hash_helper<T>{}(v); | ||
| return hash_helper<T>::hash(v); |
There was a problem hiding this comment.
Why we need hash-related change in this file?
There was a problem hiding this comment.
These changes was used to simplify the implementation of partial specialization hash_helper for tuple-like containers (Eg. std::tuple/std::pair), and generalize the partial specialization hash_helper for iterable containers(Eg. std::set).
There was a problem hiding this comment.
We intentionally don't make it a general hasher so that nobody can use low-perf iterable containers for cache key. For all list-like cases, we should always use std::tuple; for all set-like cases, we should always use std::set.
There was a problem hiding this comment.
If we want to restrict developers to using only std::set and std::tuple, then we should be able to remove the entire hash_helper and only add a std::hash specialization.
There was a problem hiding this comment.
We don't want to override the default hasher for any type, so we do not use std::hash specialization. We just define a common::op::hash custom hash functor and use it in std::unordered_map<CacheKey, std::shared_ptr<AttentionOp>, hash<CacheKey>> op_cache; in cpp/tensorrt_llm/thop/attentionOp.cpp to use it.
There was a problem hiding this comment.
Add two custom specialization of std::hash, then we don't need an extra hash_helper/hash classes.
Signed-off-by: Yihan Wang <yihwang@nvidia.com>
9819e12 to
d98d0b5
Compare
Signed-off-by: Yihan Wang <yihwang@nvidia.com>
1ed6ebd to
9819e12
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/tensorrt_llm/common/opUtils.h (1)
146-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required C++ naming convention.
Rename
hash_combinetohashCombineandseedto ak-prefixed camel-case constant name. As per coding guidelines: “local variables/functions/namespaces in lower camelCase” and constants use “thek-prefixed camelCase names.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tensorrt_llm/common/opUtils.h` around lines 146 - 150, Rename the function hash_combine to hashCombine and update every reference to it; rename the local constant seed to a k-prefixed camelCase name such as kSeed while preserving its constexpr value and hashing behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tensorrt_llm/common/opUtils.h`:
- Around line 156-185: Remove the std::hash<std::set<T>> and
std::hash<std::tuple<Args...>> specializations from opUtils.h. Introduce or
reuse project-owned hash functors for these types, then update the mObservers
unordered_map declaration in opUtils.cpp to pass the appropriate hash functor
explicitly while preserving its existing hashing behavior.
---
Nitpick comments:
In `@cpp/tensorrt_llm/common/opUtils.h`:
- Around line 146-150: Rename the function hash_combine to hashCombine and
update every reference to it; rename the local constant seed to a k-prefixed
camelCase name such as kSeed while preserving its constexpr value and hashing
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e4806ae3-e58d-4e52-8762-d4839fa92f99
📒 Files selected for processing (2)
cpp/tensorrt_llm/common/opUtils.cppcpp/tensorrt_llm/common/opUtils.h
Signed-off-by: Yihan Wang <yihwang@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tensorrt_llm/common/opUtils.h (1)
115-120: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueApply the repository naming and const conventions.
Rename
hash_combine/hash_implto lower camel case,hash_valuetohashValue, andseedto ak-prefixed constant such askHashCombineSeed. The copied-but-unmodifiedhashparameter should also beconst.Proposed adjustment
-inline size_t hash_combine(size_t hash, T const& value) +inline size_t hashCombine(size_t const hash, T const& value) { - static constexpr size_t seed = 0x9e3779b9ULL; + static constexpr size_t kHashCombineSeed = 0x9e3779b9ULL;As per coding guidelines, “local variables/functions/namespaces [use] lower camelCase,” unmodified variables are
const, and constants use thek-prefixed convention.Also applies to: 128-132, 141-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/tensorrt_llm/common/opUtils.h` around lines 115 - 120, Apply the naming and const conventions consistently across hash_combine, hash_impl, and hash_value: rename them to lower camel case (hashCombine, hashImpl, and hashValue), make the unmodified hash parameter const, and rename the seed constant to a k-prefixed name such as kHashCombineSeed. Update all references in the affected hash utilities accordingly.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/tensorrt_llm/common/opUtils.h`:
- Around line 115-120: Apply the naming and const conventions consistently
across hash_combine, hash_impl, and hash_value: rename them to lower camel case
(hashCombine, hashImpl, and hashValue), make the unmodified hash parameter
const, and rename the seed constant to a k-prefixed name such as
kHashCombineSeed. Update all references in the affected hash utilities
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee7bfc6f-e51c-45b6-84ea-19d3131703ec
📒 Files selected for processing (3)
cpp/tensorrt_llm/common/opUtils.cppcpp/tensorrt_llm/common/opUtils.hcpp/tensorrt_llm/thop/attentionOp.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/tensorrt_llm/thop/attentionOp.cpp
|
/bot run --disable-fail-fast |
|
PR_Github #61532 [ run ] triggered by Bot. Commit: |
|
|
||
| namespace | ||
| { | ||
| using tensorrt_llm::common::op::hash; |
There was a problem hiding this comment.
Move the new using tensorrt_llm::common::op::OpCustomHash; here inside anonymous namespace.
| // CUDA resources are per-context and per-thread. | ||
| using CacheKey = std::tuple<CUcontext, std::thread::id>; | ||
| std::unordered_map<CacheKey, std::weak_ptr<T>, hash<CacheKey>>* mObservers; | ||
| using CacheTy = std::unordered_map<CacheKey, std::weak_ptr<T>, OpCustomHash<CacheKey>>; |
There was a problem hiding this comment.
Use CacheType instead of CacheTy
5 participants
Summary by CodeRabbit
Bug Fixes / Improvements
cpp/tensorrt_llm/common/opUtils.hby removing the prior internalhash_helper/common::op::hashmechanism and introducing an internalOpCustomHash(anonymous namespace) with:hash_combinehelperstd::set<T>andstd::tuple<Args...>(implemented viaOpCustomHash+ index expansion)std::hash<T>cpp/tensorrt_llm/thop/attentionOp.cpp:static std::shared_mutex op_cache_mutextry_emplacehash<CacheKey>toOpCustomHash<CacheKey>cpp/tensorrt_llm/common/opUtils.cpp:std::unique_ptr<CacheTy>(was raw pointer with manualnew/delete)OpCustomHash<CacheKey>Dev Engineer Review
try_emplaceflow. On a miss, a thread may still doop->initialize()/runner->prepare(*op)before acquiring the exclusive lock; if another thread inserted first, the preparedopis discarded—confirm this is acceptable for performance and does not cause unintended side effects.attention_supports_nvfp4_output(...)still uses a function-localstatic std::unordered_map<..., OpCustomHash<...>> op_cacheand writes viaop_cache[cache_key] = ...without any visible locking. If this function can be called concurrently, this is a potential remaining data race / concurrent modification risk.OpCustomHashforstd::setandstd::tuplerelies onhash_combineand XOR-based accumulation. Confirm this satisfies any project expectations around hash consistency (across runs if needed) and that it works correctly for the actual cache key types used.opUtils.cpp, the weak-observer deleter lambda capturesthisand accessesmObservers. Double-check that cachedshared_ptrinstances cannot outlive thePerCudaCtxPerThreadSingletonCreatorobject during shutdown/static destruction (to avoid use-after-free).QA Engineer Review
No test changes.
Description
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.