-
Notifications
You must be signed in to change notification settings - Fork 759
gui: limit number of bpins to avoid excessive render times #9194
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
gui: limit number of bpins to avoid excessive render times #9194
Conversation
Signed-off-by: Peter Gadfort <[email protected]>
Signed-off-by: Peter Gadfort <[email protected]>
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.
Code Review
This pull request introduces a limit on the number of block pins (bpins) to render per layer, which is a valuable performance enhancement for designs with a high density of pins. The core logic is sound, but I've identified a performance issue in how the number of bpins is counted, specifically involving a two-pass approach which is less efficient. As per repository guidelines on performance, a single-pass approach with early exit is preferred. The new size() method on Search::Range has O(N) complexity, and using it before iterating again results in two passes over the data. I've provided a more efficient alternative in my comments. Additionally, I've noted that the name size() can be misleading for a method with O(N) complexity. The other changes are good refactorings.
| auto bpins = viewer_->search_.searchBPins( | ||
| block, layer, bounds.xMin(), bounds.yMin(), bounds.xMax(), bounds.yMax()); | ||
| debugPrint(logger_, | ||
| GUI, | ||
| "draw", | ||
| 2, | ||
| "found {} bpins on layer {}, drawing limit {} ({}) bpins", | ||
| bpins.size(), | ||
| layer->getName(), | ||
| max_bpins, | ||
| max_lin_bpins); | ||
| if (bpins.size() > max_bpins) { | ||
| return; | ||
| } |
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.
This implementation is inefficient for a large number of bpins. bpins.size() iterates through all the search results to count them (an O(N) operation for the forward iterators used here), and then the for loop iterates over them again to draw them. This results in two passes over the data.
A more efficient approach is to iterate once, collecting the bpins into a vector and stopping as soon as the number of bpins exceeds max_bpins. This avoids a full second pass over the data when the limit is exceeded. This is especially important as this change is intended to handle cases with a very large number of bpins.
auto bpins_range = viewer_->search_.searchBPins(
block, layer, bounds.xMin(), bounds.yMin(), bounds.xMax(), bounds.yMax());
std::vector<std::pair<odb::dbBox*, odb::dbBPin*>> bpins;
bpins.reserve(max_bpins + 1);
for (const auto& bpin : bpins_range) {
bpins.push_back(bpin);
if (bpins.size() > max_bpins) {
break;
}
}
debugPrint(logger_,
GUI,
"draw",
2,
"found {} bpins on layer {}, drawing limit {} ({}) bpins",
bpins.size() > max_bpins ? std::string(">") + std::to_string(max_bpins) : std::to_string(bpins.size()),
layer->getName(),
max_bpins,
max_lin_bpins);
if (bpins.size() > max_bpins) {
return;
}References
- When adjusting font size for multiple labels, on-the-fly adjustment while iterating can be more efficient than a two-pass approach (find max length, then adjust). The single-pass method allows for early exit if minimum font size is reached, avoiding processing all labels.
| Iterator begin() { return begin_; } | ||
| Iterator end() { return end_; } | ||
|
|
||
| int size() const { return std::distance(begin_, end_); } |
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.
The size() method has a time complexity of O(N), where N is the number of elements in the range, because it uses std::distance on forward iterators. This can be inefficient if the range is large. For collections where size() is not an O(1) operation, it's a common C++ practice to name the method count() to avoid giving a false impression of its performance. Using this size() method and then iterating over the range again results in two passes over the data, which is inefficient for large datasets.
References
- When adjusting font size for multiple labels, on-the-fly adjustment while iterating can be more efficient than a two-pass approach (find max length, then adjust). The single-pass method allows for early exit if minimum font size is reached, avoiding processing all labels.
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.
clang-tidy made some suggestions
Signed-off-by: Peter Gadfort <[email protected]>
|
clang-tidy review says "All clean, LGTM! 👍" |
Changes:
Notes: