-
Notifications
You must be signed in to change notification settings - Fork 259
search: skip redundant binary search on tag insert #382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -168,6 +168,8 @@ TagGroupBldr::init(Vertex *vertex) | |
| vertex_ = vertex; | ||
| path_index_map_.clear(); | ||
| paths_.clear(); | ||
| insert_pos_ = no_insert_pos_; // OpenROAD-fork: tag-match | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| insert_tag_ = nullptr; | ||
| has_clk_tag_ = false; | ||
| has_genclk_src_tag_ = false; | ||
| has_filter_tag_ = false; | ||
|
|
@@ -198,13 +200,22 @@ TagGroupBldr::tagMatchPath(Tag *tag, | |
| // Find matching group tag. | ||
| // Match is not necessarily equal to original tag because it | ||
| // must only satisfy tagMatch. | ||
| bool exists; | ||
| findKeyValue(path_index_map_, tag, path_index, exists); | ||
| if (exists) | ||
| // OpenROAD-fork: tag-match | ||
| // findInsertHint() records the sorted insertion position so a following | ||
| // insertPath() (when match == nullptr) can skip a second binary search. | ||
| size_t pos; | ||
| auto it = path_index_map_.findInsertHint(tag, pos); | ||
| if (it != path_index_map_.end()) { | ||
| path_index = it->second; | ||
| match = &paths_[path_index]; | ||
| insert_pos_ = no_insert_pos_; | ||
| insert_tag_ = nullptr; | ||
| } | ||
| else { | ||
| match = nullptr; | ||
| path_index = 0; | ||
| insert_pos_ = pos; | ||
| insert_tag_ = tag; | ||
| } | ||
|
Comment on lines
+206
to
219
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Record the size_t pos;
auto it = path_index_map_.findInsertHint(tag, pos);
if (it != path_index_map_.end()) {
path_index = it->second;
match = &paths_[path_index];
insert_pos_ = no_insert_pos_;
insert_tag_ = nullptr;
}
else {
match = nullptr;
path_index = 0;
insert_pos_ = pos;
insert_tag_ = tag;
} |
||
| } | ||
|
|
||
|
|
@@ -258,7 +269,19 @@ TagGroupBldr::insertPath(Tag *tag, | |
|
|
||
| { | ||
| size_t path_index = paths_.size(); | ||
| path_index_map_[tag] = path_index; | ||
| // OpenROAD-fork: tag-match | ||
| // Reuse the insertion position recorded by the immediately preceding | ||
| // tagMatchPath() (which found no match) to avoid a redundant binary search. | ||
| // The hint is only valid when no map mutation happened in between, which is | ||
| // the case for all setArrival()/setMatchPath() call sites. Fall back to a | ||
| // full sorted insert otherwise. | ||
| if (insert_pos_ != no_insert_pos_ && insert_tag_ == tag) { | ||
| path_index_map_.insertAtPos(insert_pos_, tag, path_index); | ||
| insert_pos_ = no_insert_pos_; | ||
| insert_tag_ = nullptr; | ||
| } | ||
| else | ||
| path_index_map_[tag] = path_index; | ||
| paths_.emplace_back(vertex_, tag, arrival, prev_path, prev_edge, prev_arc, sta_); | ||
|
|
||
| if (tag->isClock()) | ||
|
|
@@ -276,6 +299,11 @@ TagGroupBldr::insertPath(Tag *tag, | |
| void | ||
| TagGroupBldr::insertPath(const Path &path) | ||
| { | ||
| // OpenROAD-fork: tag-match | ||
| // This overload has no preceding tagMatchPath(), so clear any stale hint to | ||
| // force a full sorted insert. | ||
| insert_pos_ = no_insert_pos_; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| insert_tag_ = nullptr; | ||
| insertPath(path.tag(sta_), path.arrival(), path.prevPath(), path.prevEdge(sta_), | ||
| path.prevArc(sta_)); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -153,6 +153,15 @@ protected: | |
| int default_path_count_; | ||
| PathIndexMap path_index_map_; | ||
| std::vector<Path> paths_; | ||
| // OpenROAD-fork: tag-match | ||
| // Insertion position recorded by tagMatchPath() when a tag is absent, so the | ||
| // following insertPath() can skip a redundant binary search. no_insert_pos_ | ||
| // means "no valid hint" (callers that insert without a preceding lookup). | ||
| static constexpr size_t no_insert_pos_ = ~static_cast<size_t>(0); | ||
| size_t insert_pos_{no_insert_pos_}; | ||
|
Comment on lines
+160
to
+161
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Storing only the insertion index ( To make this robust and defensively programmed, we should also store the static constexpr size_t no_insert_pos_ = ~static_cast<size_t>(0);
size_t insert_pos_{no_insert_pos_};
Tag *insert_tag_{nullptr}; |
||
| // Tag the hint was recorded for; the hint is only reused when the following | ||
| // insertPath() is for this same tag (guards against a stale hint). | ||
| Tag *insert_tag_{nullptr}; | ||
| bool has_clk_tag_{false}; | ||
| bool has_genclk_src_tag_{false}; | ||
| bool has_filter_tag_{false}; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,125 @@ | ||
| // OpenSTA, Static Timing Analyzer | ||
| // Copyright (c) 2026, Parallax Software, Inc. | ||
| // | ||
| // This program is free software: you can redistribute it and/or modify | ||
| // it under the terms of the GNU General Public License as published by | ||
| // the Free Software Foundation, either version 3 of the License, or | ||
| // (at your option) any later version. | ||
| // | ||
| // This program is distributed in the hope that it will be useful, | ||
| // but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| // GNU General Public License for more details. | ||
| // | ||
| // You should have received a copy of the GNU General Public License | ||
| // along with this program. If not, see <https://www.gnu.org/licenses/>. | ||
|
|
||
| // OpenROAD-fork: tag-match | ||
| // Unit test for the VectorMap insert-position hint API | ||
| // (findInsertHint / insertAtPos) added to skip the redundant binary search | ||
| // on the TagGroupBldr tag-insert hot path. Verifies that building a map with | ||
| // the hint API yields exactly the same contents and order as operator[]. | ||
|
|
||
| #include "VectorMap.hh" | ||
|
|
||
| #include <random> | ||
| #include <vector> | ||
|
|
||
| #include "gtest/gtest.h" | ||
|
|
||
| namespace { | ||
|
|
||
| using sta::VectorMap; | ||
|
|
||
| using IntMap = VectorMap<int, int, std::less<int>>; | ||
|
|
||
| // Build a reference map with operator[] and a candidate map with the | ||
| // findInsertHint()/insertAtPos() fast path, then assert identical contents. | ||
| static void | ||
| checkHintMatchesOperatorBracket(const std::vector<int> &keys) | ||
| { | ||
| IntMap ref{std::less<int>()}; | ||
| IntMap hint{std::less<int>()}; | ||
|
|
||
| for (int k : keys) { | ||
| int value = k * 10 + 1; | ||
| // Reference: plain operator[] insert (does its own binary search). | ||
| ref[k] = value; | ||
|
|
||
| // Candidate: mimic TagGroupBldr's tagMatchPath()+insertPath() pattern. | ||
| size_t pos = 0; | ||
| auto it = hint.findInsertHint(k, pos); | ||
| if (it != hint.end()) | ||
| // Already present: overwrite, same as operator[]. | ||
| (*it).second() = value; | ||
| else | ||
| hint.insertAtPos(pos, k, value); | ||
| } | ||
|
|
||
| ASSERT_EQ(ref.size(), hint.size()); | ||
| // VectorMap iterates in sorted key order; compare element by element. | ||
| auto ri = ref.begin(); | ||
| auto hi = hint.begin(); | ||
| for (; ri != ref.end() && hi != hint.end(); ++ri, ++hi) { | ||
| EXPECT_EQ((*ri).first(), (*hi).first()); | ||
| EXPECT_EQ((*ri).second(), (*hi).second()); | ||
| } | ||
| EXPECT_EQ(ri, ref.end()); | ||
| EXPECT_EQ(hi, hint.end()); | ||
| } | ||
|
|
||
| TEST(VectorMapHint, InsertAscending) | ||
| { | ||
| checkHintMatchesOperatorBracket({0, 1, 2, 3, 4, 5, 6, 7}); | ||
| } | ||
|
|
||
| TEST(VectorMapHint, InsertDescending) | ||
| { | ||
| checkHintMatchesOperatorBracket({7, 6, 5, 4, 3, 2, 1, 0}); | ||
| } | ||
|
|
||
| TEST(VectorMapHint, InsertWithDuplicates) | ||
| { | ||
| // Duplicate keys must take the "found" branch and overwrite, not reinsert. | ||
| checkHintMatchesOperatorBracket({3, 1, 3, 2, 1, 0, 2, 3, 5, 5}); | ||
| } | ||
|
|
||
| TEST(VectorMapHint, InsertRandomOrders) | ||
| { | ||
| std::mt19937 rng(12345); | ||
| for (int trial = 0; trial < 200; trial++) { | ||
| std::vector<int> keys; | ||
| int n = rng() % 30; | ||
| for (int i = 0; i < n; i++) | ||
| keys.push_back(static_cast<int>(rng() % 20)); | ||
| checkHintMatchesOperatorBracket(keys); | ||
| } | ||
| } | ||
|
|
||
| TEST(VectorMapHint, HintFindAgreesWithFind) | ||
| { | ||
| IntMap m{std::less<int>()}; | ||
| for (int k : {10, 20, 30, 40}) { | ||
| size_t pos; | ||
| auto it = m.findInsertHint(k, pos); | ||
| EXPECT_EQ(it, m.end()); | ||
| m.insertAtPos(pos, k, k); | ||
| } | ||
| // Present keys: findInsertHint must agree with find(). | ||
| for (int k : {10, 20, 30, 40}) { | ||
| size_t pos; | ||
| auto hint_it = m.findInsertHint(k, pos); | ||
| auto find_it = m.find(k); | ||
| ASSERT_NE(hint_it, m.end()); | ||
| EXPECT_EQ((*hint_it).second(), (*find_it).second()); | ||
| } | ||
| // Absent keys: end() and a valid insertion position keeping order sorted. | ||
| for (int k : {5, 15, 25, 35, 45}) { | ||
| size_t pos; | ||
| auto hint_it = m.findInsertHint(k, pos); | ||
| EXPECT_EQ(hint_it, m.end()); | ||
| EXPECT_LE(pos, m.size()); | ||
| } | ||
| } | ||
|
|
||
| } // namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To prevent silent corruption of the sorted map structure, it is highly recommended to add assertions in
insertAtPosto verify that the insertion index is within bounds and that inserting the key atpospreserves the strict weak ordering of the map. SinceVectorMaprelies on being sorted for binary search, any out-of-order insertion will cause subsequent lookups to fail silently.Note: This requires including
<cassert>at the top of the file if it is not already transitively included.