add MT info to tag doc#387
Conversation
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
![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 updates the doc/TagsAndScenes.md documentation to add a detailed section on threading and concurrency in OpenSTA, explaining the thread pool, parallel phases, level-barrier invariants, and locking mechanisms. The reviewer pointed out a critical concurrency bug in the referenced implementation of Search::findTagGroup where a self-assignment prevents the atomic pointer tag_groups_ from being updated, which violates the documented design.
| pointers are atomic (`std::atomic<Tag**> tags_`, `std::atomic<TagGroup**> | ||
| tag_groups_`, `include/sta/Search.hh:632,641`) and growth is copy-then-publish: | ||
| under the table lock, a double-size array is allocated, existing entries are | ||
| copied, and only then is the atomic pointer swung | ||
| (`search/Search.cc:2919-2930,2659-2670` — "make the new array and copy the | ||
| contents into it before updating tags_ so that other threads can use | ||
| Search::tag(TagIndex) without returning gubbish"). The retired arrays are |
There was a problem hiding this comment.
While the documentation correctly describes the intended copy-then-publish design, there is a critical concurrency bug in the referenced code at Search::findTagGroup (search/Search.cc:2667). In Search::findTagGroup, the local variable tag_groups is self-assigned (tag_groups = tag_groups;) instead of updating the member variable tag_groups_ (tag_groups_ = tag_groups;). As a result, the atomic pointer tag_groups_ is never actually updated when the table grows, which violates the thread-safety invariant described here and can lead to out-of-bounds reads or stale data. It is highly recommended to submit a separate fix for search/Search.cc:2667 to correctly update tag_groups_.
There was a problem hiding this comment.
Hallucination. Line 2667 reads tag_groups_ = tag_groups; - member (trailing underscore) assigned from local. No self-assignment. Atomic pointer updated correctly, same pattern as tags_ grow in findTag (Search.cc:2927). Also, self-assignment of a local would trigger -Wself-assign warnings and the doc'd invariant would break visibly in any multi-thread run with >128 tag groups - it doesn't.
You likely misread the two identifiers differing only by trailing underscore.
2 participants
Added a section on how threading is handled to the tag and scene doc as requested in #386