[Dyninst/Rocprofiler-systems]:Removal of TBB dependency - #26
[Dyninst/Rocprofiler-systems]:Removal of TBB dependency#26sputhala-amd wants to merge 11 commits into
Conversation
…dered_set First step of removing the oneTBB dependency from Dyninst (the low-risk items): - DyninstTBB.cmake: drop the tbbmalloc and tbbmalloc_proxy components. These are a link-time global allocator override with no source-level API usage; Dyninst falls back to the system allocator. - concurrent.h: remove the unused dyn_c_queue alias and the <tbb/concurrent_queue.h> include (no consumers anywhere in the tree). - indexed_modules.h: replace tbb::concurrent_unordered_set with a std::unordered_set guarded by a shared_mutex. The module index has small cardinality and is populated during parsing then read afterwards, so mutations take an exclusive lock and lookups take a shared lock. concurrent_hash_map and concurrent_vector are still provided by TBB and are handled in later commits.
…c_vector Replace tbb::concurrent_vector (the dyn_c_vector alias) with a small std::deque-derived class that preserves the two properties Dyninst relies on: - Concurrent push_back/emplace_back: serialized by an internal mutex. Parser.C appends to a shared dyn_c_vector from inside an OpenMP parallel loop, so the mutation entry points must be thread-safe. - Stable element addresses: std::deque never relocates elements on growth, so pointers/references handed out (e.g. Type::getComponents) stay valid, matching concurrent_vector's guarantee. Element access is inherited from std::deque and left unlocked; callers append during the parallel phase and read afterwards. No concurrent_vector-specific API (grow_by, range, reserve, capacity) is used and push_back's return value is never consumed, so std::deque covers the accessed surface. Also include <tbb/version.h> explicitly (previously pulled in transitively via concurrent_vector.h) so dyn_c_hash_map's TBB_VERSION_MAJOR check still resolves.
…ed_map Final step of removing the oneTBB dependency from Dyninst. Replaces tbb::concurrent_hash_map (the dyn_c_hash_map wrapper) with a sharded std::unordered_map, after which no Dyninst source uses TBB. - concurrent.h: dyn_c_hash_map now partitions keys across a fixed number of shards, each an independent std::unordered_map guarded by its own shared_mutex. The accessor/const_accessor interface is preserved -- an accessor holds its key's shard exclusively, a const_accessor holds it shared -- so the call sites are unchanged. Dyninst never holds two accessors into the same map instance simultaneously, so per-shard locking cannot self-deadlock. std::shared_mutex is understood natively by Valgrind DRD/Helgrind, so the explicit lock annotations are no longer needed. The last <tbb/*> includes and the hash_compare shim are removed. - IBSTree-fast.h, emitElf.h, dwarfWalker.C: add <iterator>/<climits> includes that were previously pulled in transitively through the TBB headers. - Build: drop the now-unused TBB dependency -- remove include(DyninstTBB) from CMakeLists.txt and DyninstConfig.cmake.in, drop Dyninst::TBB from common's PUBLIC_DEPS, delete cmake/tpls/DyninstTBB.cmake, and remove the TBB version check from the dependency-version workflow.
There was a problem hiding this comment.
Pull request overview
Replaces oneTBB-backed containers with synchronized standard-library implementations and removes related build integration.
Changes:
- Adds sharded hash-map and mutex-protected deque wrappers.
- Replaces the module index with a locked
std::unordered_set. - Removes TBB CMake and dependency-version wiring.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
common/h/concurrent.h |
Implements standard-library concurrent wrappers. |
common/h/IBSTree-fast.h |
Adds required iterator include. |
symtabAPI/src/indexed_modules.h |
Replaces TBB module index. |
symtabAPI/src/emitElf.h |
Adds limits include. |
symtabAPI/src/dwarfWalker.C |
Adds limits include. |
common/CMakeLists.txt |
Removes TBB linkage. |
CMakeLists.txt |
Removes TBB discovery. |
cmake/tpls/DyninstTBB.cmake |
Deletes TBB configuration. |
cmake/DyninstConfig.cmake.in |
Removes exported TBB discovery. |
.github/workflows/dependency-version.yaml |
Removes TBB version validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Combines the fixes for the concurrent_hash_map review comments: - Publish the std::iterator_traits member aliases on the sharded dyn_c_hash_map iterators (iterator_category, value_type, difference_type, pointer, reference), deriving iterator_category/difference_type from the wrapped std::unordered_map iterator via std::iterator_traits. Without them the iterators were unusable with std::iterator_traits and standard algorithms -- a regression from the TBB iterator. Also adds the <iterator> include. - Remove the racy erase(const_accessor&) overload. It released the accessor's shared lock and re-acquired an exclusive lock by key, so the erase was no longer tied to the referenced element (another thread could erase/reinsert the key in the gap and the wrong element would be removed). It had no callers; callers use erase(accessor&) (atomic under the shard's exclusive lock) or erase(const K&).
sputhala-amd
changed the title
kcossett-amd
left a comment
kcossett-amd
left a comment
There was a problem hiding this comment.
Leaving a comment that my AI flagged to me whilst reviewing:
Blocking: dyn_c_hash_map shard locking breaks Parser::set_edge_parsing_status
dyn_c_hash_map changed its locking granularity from per element (TBB) to per shard (64 shards, hash(key) % 64), and an accessor now holds that shard lock for its entire lifetime. Any thread that holds two accessors into the same map instance therefore self-deadlocks as soon as the two keys land in the same shard.
Parser::set_edge_parsing_status holds up to four accessors on one map instance, so
ParseAPI now aborts with Resource deadlock avoided on most non-trivial binaries.
Evidence
a1 is acquired at Parser.C:2463 and lives to the end of the function, while a2/a3/a4 hit the same edm at 2509, 2586, 2592. Keys are code addresses and std::hash is identity for integers, so it fails whenever addr % 64 == A->last() % 64.
Same ParseAPI driver, two builds of this tree — baseline 07b585a7f (TBB) passes 59/59, branch 6bbc5abd8 fails 10/59. /bin/ls, /bin/bash, libstdc++.so.6, python3.12, git and gdb all abort here and parse fine on baseline. It reproduces at OMP_NUM_THREADS=1, so this is a deterministic logic error, not a race.
GDB on /bin/ls — the aborting thread is self-deadlocked, with the other three workers stalled on that same rwlock:
what(): Resource deadlock avoided
#14 dyn_c_hash_map<...>::insert at common/h/concurrent.h:225
#15 Parser::set_edge_parsing_status at parseAPI/src/Parser.C:2509
Both modes exist: write-then-anything aborts, read-then-write hangs silently. Also note the comment at concurrent.h:77-80 asserts the invariant that this violates.
Suggested fix: restore per-element locking — a mutex per node held by the accessor, with the shard lock used only for the lookup. std::unordered_map is node-based so element addresses are stable. That matches TBB's contract and needs no call-site changes. Raising the shard count only makes the bug rarer, and reworking the call site is delicate since it mutates through a1 after a2 exists (2503-2504, 2575-2576).
The sharded dyn_c_hash_map held a shard lock for the accessor's whole
lifetime, so a thread holding two accessors into one map (e.g.
Parser::set_edge_parsing_status) self-deadlocked whenever two keys shared a
shard ("Resource deadlock avoided", or a silent hang for read-then-write).
Restore TBB's per-element locking: each element owns a shared_mutex held by
the accessor, with the shard lock used only for the structural lookup and
always released before the element lock (no nesting). Elements are held via
shared_ptr so a concurrent erase cannot destroy a locked node. Newly-created
nodes are locked while the shard lock is still held (atomic insert-and-lock),
so no thread can observe an element before the inserter initializes it --
without this, a second thread read an uninitialized value and crashed in
Symtab::addSymbolToAggregates.
Co-authored-by: Cursor <cursoragent@cursor.com>
Address residual issues found in a follow-up review of the TBB-container replacements: dyn_c_hash_map (Finding 1/4): restore tbb::concurrent_hash_map's atomic find/insert-and-lock and erase semantics that were lost when the element lock was taken after the shard lock was released. - find/insert now validate-after-lock: after acquiring an existing element's lock (shard lock already dropped), re-check under the shard lock that the key still maps to that same node; if it was erased/replaced, retry. Prevents binding an accessor to a stale node under concurrent erase+reinsert. - erase now acquires the element lock first (so it waits for outstanding accessors, like TBB) before removing under the shard lock; erase(accessor) removes by node identity rather than by key. Newly-created nodes are still locked under the shard lock (no gap), so no validation is needed there. dyn_c_vector (Finding 2/3): inherit std::deque privately and re-export only the used API, so a dyn_c_vector can no longer be sliced to or bound as a std::deque& (which would bypass the append lock) and cannot be deleted through a std::deque*. Document the concurrency contract: only concurrent append is synchronized; element access and non-append mutation must be phase-separated from appends. Verified: full dyninstAPI build; instrument ls/bash/python3 single- and multi-threaded (128 threads); ThreadSanitizer stress test of dyn_c_hash_map (0 data races, no deadlock). Co-authored-by: Cursor <cursoragent@cursor.com>
Thanks @kcossett-amd. Pushed fixes. Ran Tsan checks this time. Please rev-review. |
dyn_c_hash_map picked a shard with `std::hash(k) % num_shards`. In libstdc++ std::hash is the identity for pointers and integers, and Dyninst's keys are dominated by heap pointers (CodeRegion*, Block*, Function*, void*) and function entry addresses, all of which are 16-byte aligned. Their low four bits are therefore always zero, so a power-of-two modulo could only ever reach every 16th shard: 4 of 64, 16 of 256, 64 of 1024. The whole parallel parse was consequently funneling through a handful of shard mutexes, which is what made throughput fall as threads were added rather than rise. oneTBB did not have this problem because tbb_hash_compare multiplies the key by a hash multiplier before use. Run the hash through a MurmurHash3 finalizer before the modulo so every shard is reachable. Measured with a mixed find/insert benchmark on 16-byte-aligned keys (Mops/s at 16/32/64 threads): 256 shards, before: 5.1 / 4.0 / 4.0 1024 shards, before: 17.6 / 17.1 / 18.5 256 shards, after: 29.6 / 32.8 / 50.5 That is roughly 8x at 32 threads, and scaling is positive again. It also means 256 shards now outperforms 1024 unmixed while using a quarter of the memory. Verified: clean build with no new warnings; instrument ls/bash/python3 at 1 and 128 threads with unchanged results; ThreadSanitizer stress test of dyn_c_hash_map reports no data races or lock-order inversions.
tbb::concurrent_vector allowed one thread to read elements while another appended. The std::deque-backed dyn_c_vector did not: push_back can reallocate the deque's internal map array out from under a reader that is walking it to locate an element, so a concurrent indexed read could follow a freed pointer. Dyninst relies on the old guarantee. fieldListType::operator== reads fieldList.size() and fieldList[i] with no lock, and addOrUpdateType calls it on a type that another OpenMP worker may still be filling in, because parseStructUnionClass publishes the type into typesByID before parsing its members. Take the append mutex in operator[], at, front, back, size and empty. Releasing it before the caller uses the returned reference is safe because std::deque never relocates existing elements: only the traversal that locates the element needs protecting, not the element itself. Iteration (begin/end/rbegin/rend) remains unsynchronized, since push_back invalidates every deque iterator and no caller iterates concurrently with an append. The class-level concurrency contract is updated to state exactly which operations are now safe against a concurrent append. Verified: clean build with no new warnings; instrument ls/bash/python3 at 1 and 128 threads, all succeeding with a stable instrumented-function set.
|
@kcossett-amd - Regarding #26 (comment), I will have to revert The shard array is allocated eagerly in the constructor, so a map that never receives an element still pays in full. Measuring the
Measured peak RSS at 8 threads against a stock TBB build of the same commit's parent:
Reverting to 64 roughly halves the remaining memory regression. Why 64 looked bad in your benchmark.
So the 0.37 s -> 0.83 s cliff at 64 shards was really 4 shards' worth of contention, and 1024 restored oneTBB performance because it was the first value that yielded 64 effective shards. Commit 84e2c4c fixes the issue, so all shards are now reachable and 64 mixed shards gives the same effective parallelism your 1024 recommendation bought - at 1/16 the memory. |
The shard array is allocated eagerly, so every map instance pays 112 bytes per shard regardless of how many elements it holds. Dyninst keeps thousands of these maps alive during a parse -- roughly 5500 while instrumenting a 1 MB binary -- so at 256 shards the empty arrays alone account for ~158 MB, and they dominate the actual element data on small and medium targets. 256 was chosen because 64 scaled negatively past 16 threads, but that was the unmixed hash rather than the shard count. Keys are dominated by 16-byte aligned addresses whose low four bits are constant, so `% 64` reached only 4 distinct shards and `% 1024` only 64 -- which is why 1024 appeared to restore oneTBB performance. Since 84e2c4c avalanches the hash before the modulo, 64 shards yields 64 reachable shards, matching the effective parallelism of the earlier 1024 recommendation at a sixteenth of the memory. Measured on a full parse of libdyninstAPI.so (68 MB, 109/109 modules), 64 shards is both faster and smaller than 256 at every thread count from 1 to 128, with peak speedup unchanged at 1.40x on 8 threads. Peak RSS at 8 threads drops from 617 to 504 MB on a 1 MB target and from 3335 to 3208 MB on the 68 MB target. Function lists are unchanged.
sputhala-amd
changed the title
| bool r = base::find(a, k); | ||
| if(r) a.acquire(); | ||
| return r; | ||
| a.release(); |
There was a problem hiding this comment.
find() calculates the shard twice: once in find_node() and again in still_current(). Please calculate shard_for(k) once and pass the shard reference to both helpers.
| } | ||
|
|
||
| int contains(const K& k) { return base::count(k) == 1; } | ||
| int contains(const K& k) const { return find_node(k) != nullptr; } |
There was a problem hiding this comment.
contains calls find_node, which copies a shared_ptr and therefore performs an atomic reference-count increment and decrement on a successful lookup.
Motivation
This PR removes oneTBB entirely, replacing each container with a standard-library equivalent, which drops a submodule/find_package requirement, simplifies the build, and shrinks the dependency surface with no change to the parallelism model.
TBB is used exclusively for concurrent containers + the scalable allocator; the actual parallelism is OpenMP-driven. TBB's scheduler/algorithm side is entirely unused (zero parallel_for/task_group/flow-graph).
Everything is encapsulated in one header (common/h/concurrent.h) via dyn_c_* wrappers, plus one direct use in symtabAPI/src/indexed_modules.h.
Dyninst uses OpenMP for its internal parallelism for parsing binaries (#pragma omp parallel for/task in parseAPI/symtabAPI). The OpenMP threads write concurrently into shared structures → those structures are the TBB concurrent containers.
Mitigating factor: Dyninst's parse is a one-shot instrumentation-time cost, not per-sample profiling overhead — so even a parse-time regression has bounded impact.
tbbmalloc/tbbmalloc_proxyconcurrent_queuedyn_c_queueconcurrent_unordered_setindexed_modules.h, direct use)std::unordered_set+std::shared_mutex(tiny cardinality — modules, not symbols; phase-separated; no erase)concurrent_vectordyn_c_vectorstd::dequefor stable element addresses (notstd::vector, which reallocates → dangling pointers); perf ≈ negligible; verify single-writerconcurrent_hash_mapdyn_c_hash_mapTBB_VERSION_MAJOR>=2021shimstd::unordered_map(array of maps + per-shard mutex) — not a single mutex (would re-serialize the OpenMP parse); preserve accessor API so ~80 call sites stay unchangedTechnical Details
tbbmalloc,concurrent_queue, andconcurrent_unordered_set(low-risk)tbbmalloc/tbbmalloc_proxy- a link-time global allocator override with no source-level API use; falls back to the system allocator.dyn_c_queuealias (no consumers in the tree).concurrent_unordered_setinindexed_modules.hwithstd::unordered_set+shared_mutex(small cardinality; populated during parse, read afterwards).concurrent_vectorwith astd::deque-backeddyn_c_vectorstd::dequepreserves the two guarantees Dyninst relies on: stable element addresses on growth, and concurrentpush_back/emplace_back(serialized by an internal mutex). No concurrent_vector-specific API (grow_by,range,reserve) is used.concurrent_hash_mapwith a shardedstd::unordered_mapdyn_c_hash_mappartitions keys across a fixed number of shards, each an independentstd::unordered_mapguarded by its ownshared_mutex; an accessor holds its shard exclusively, aconst_accessorholds it shared. The accessor/const_accessor interface is preserved, so call sites are unchanged. Dyninst never holds two accessors into the same map instance at once, so per-shard locking cannot self-deadlock.std::shared_mutexis natively visible to Valgrind DRD/Helgrind, so the manual lock annotations are dropped.<iterator>/<climits>includes toIBSTree-fast.h,emitElf.h, anddwarfWalker.Cthat were previously pulled in transitively via the TBB headers.include(DyninstTBB)fromCMakeLists.txtandDyninstConfig.cmake.in,Dyninst::TBBfrom common's deps,cmake/tpls/DyninstTBB.cmake, and the TBB version check in the dependency-version workflow.Test Plan
Test Result
Submission Checklist