Sta latest 0528#367
Conversation
* fix makeConcreteParasitics leak Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com> * simplify test, update the address review comment Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com> * reduce test verbosity Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com> --------- Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
Signed-off-by: James Cherry <cherry@parallaxsw.com>
Signed-off-by: Masanori Ogino <masanori.ogino@gmail.com>
Signed-off-by: James Cherry <cherry@CerezoBook.local>
Signed-off-by: James Cherry <cherry@CerezoBook.local>
Signed-off-by: James Cherry <cherry@CerezoBook.local>
|
James Cherry seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![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 CUDD installation instructions, adds a new Homebrew TCL path for macOS, removes pointer hashing utilities, and introduces unsetAnnotatedSlew. It also fixes a memory leak in makeConcreteParasitics by freeing existing map entries before overwriting them, which is verified by a new regression test. Feedback suggests optimizing this memory leak fix by directly updating the map node pointer and utilizing transparent lookup with std::string_view to avoid unnecessary allocations and map rebalancing.
| // Free the prior entry to avoid leaking it on overwrite. | ||
| std::string key(name); | ||
| auto it = parasitics_name_map_.find(key); | ||
| if (it != parasitics_name_map_.end()) { | ||
| delete it->second; | ||
| parasitics_name_map_.erase(it); | ||
| } | ||
| Parasitics *parasitics = new ConcreteParasitics(name, filename, this); | ||
| parasitics_name_map_[std::string(name)] = parasitics; | ||
| parasitics_name_map_[key] = parasitics; | ||
| return parasitics; |
There was a problem hiding this comment.
Instead of erasing the old entry and inserting a new one, you can directly update the pointer of the existing map node. Since parasitics_name_map_ uses std::less<> as a transparent comparator, you can also search using std::string_view directly without allocating a temporary std::string key. This avoids rebalancing the map's red-black tree twice and avoids unnecessary string allocations.
auto it = parasitics_name_map_.find(name);
if (it != parasitics_name_map_.end()) {
delete it->second;
it->second = new ConcreteParasitics(name, filename, this);
return it->second;
}
Parasitics *parasitics = new ConcreteParasitics(name, filename, this);
parasitics_name_map_[std::string(name)] = parasitics;
return parasitics;There was a problem hiding this comment.
Cannot do this as it doesn't preserve the filename. It was already pointed by upstream OpenSTA developer that Gemini suggested change is incorrect
5 participants
Latest upstream OpenSTA
Note that hashPtr was removed from sta upstream (commit ffd54f3 "rm hashPtr").
So we need fix two OpenROAD callers: est/EstimateParasitics.h:47 and rsz/Resizer.hh:88 to build OpenROAD succesfully.