Debug or runtime fix#9543
Conversation
Cache the mterms on the dbInstHeader. Intended to help sta performance. Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
…only Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
|
Details in attached ppt |
![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 introduces a significant performance optimization by caching _dbMTerm pointers in _dbInstHdr, which speeds up getMTerm() lookups. The implementation of this caching mechanism appears to be thorough, with updates to object creation, serialization, and copy logic. Additionally, there's a fix in dbNetwork::libertyPort to correctly resolve liberty ports for pins, and an attempted optimization in getDbNwkObjectId. My review focuses on a potential issue with the optimization in getDbNwkObjectId where a bounds check is bypassed for common object types, which could lead to data corruption. Please see the specific comment for details.
| // Optimize for most common types to avoid switch overhead | ||
| if (typ == dbInstObj) { | ||
| return ((db_id << DBIDTAG_WIDTH) | DBINST_ID); | ||
| } | ||
| if (typ == dbNetObj) { | ||
| return ((db_id << DBIDTAG_WIDTH) | DBNET_ID); | ||
| } | ||
| if (typ == dbITermObj) { | ||
| return ((db_id << DBIDTAG_WIDTH) | DBITERM_ID); | ||
| } | ||
|
|
||
| // Bounds check only for debug/rare cases | ||
| if (db_id > (std::numeric_limits<ObjectId>::max() >> DBIDTAG_WIDTH)) { | ||
| logger_->error(ORD, 2019, "Database id exceeds capacity"); | ||
| } |
There was a problem hiding this comment.
The bounds check for db_id has been moved after the early returns for common object types (dbInstObj, dbNetObj, dbITermObj). This means that for these common types, the check is now skipped. If a large db_id is encountered for one of these types, it could lead to a silently corrupted ObjectId due to the left shift operation.
While the comment suggests this is intentional ("Bounds check only for debug/rare cases"), it introduces a risk of data corruption that was prevented by the original code. The performance gain from skipping this single if statement is likely minimal, while the risk is significant.
It is recommended to move the bounds check back to before the type-specific handling to ensure all db_id values are validated.
if (db_id > (std::numeric_limits<ObjectId>::max() >> DBIDTAG_WIDTH)) {
logger_->error(ORD, 2019, "Database id exceeds capacity");
}
// Optimize for most common types to avoid switch overhead
if (typ == dbInstObj) {
return ((db_id << DBIDTAG_WIDTH) | DBINST_ID);
}
if (typ == dbNetObj) {
return ((db_id << DBIDTAG_WIDTH) | DBNET_ID);
}
if (typ == dbITermObj) {
return ((db_id << DBIDTAG_WIDTH) | DBITERM_ID);
}
There was a problem hiding this comment.
Resolved in new version
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
| // Optimize for most common types to avoid switch overhead | ||
| if (typ == dbInstObj) { | ||
| return ((db_id << DBIDTAG_WIDTH) | DBINST_ID); | ||
| } | ||
| if (typ == dbNetObj) { | ||
| return ((db_id << DBIDTAG_WIDTH) | DBNET_ID); | ||
| } | ||
| if (typ == dbITermObj) { | ||
| return ((db_id << DBIDTAG_WIDTH) | DBITERM_ID); | ||
| } | ||
|
|
||
| switch (typ) { | ||
| case dbITermObj: { | ||
| return ((db_id << DBIDTAG_WIDTH) | DBITERM_ID); | ||
| } break; | ||
| case dbBTermObj: { | ||
| return ((db_id << DBIDTAG_WIDTH) | DBBTERM_ID); | ||
| } break; | ||
| case dbInstObj: { | ||
| return ((db_id << DBIDTAG_WIDTH) | DBINST_ID); | ||
| } break; | ||
| case dbNetObj: { | ||
| return ((db_id << DBIDTAG_WIDTH) | DBNET_ID); | ||
| } break; | ||
| case dbModITermObj: { | ||
| return ((db_id << DBIDTAG_WIDTH) | DBMODITERM_ID); | ||
| } break; |
There was a problem hiding this comment.
Since we're using c++20 you might want to give [[likely]]/[[unlikely]] a try to see if we can preserve the case switch statement to get the compiler errors in case we add a new enum.
https://en.cppreference.com/w/cpp/language/attributes/likely
switch (typ) {
[[likely]] case dbITermObj: {
return ((db_id << DBIDTAG_WIDTH) | DBITERM_ID);
} break;
[[unlikely]]case dbBTermObj: {
return ((db_id << DBIDTAG_WIDTH) | DBBTERM_ID);
} break;
case dbInstObj: {
return ((db_id << DBIDTAG_WIDTH) | DBINST_ID);
} break;
[[likely]]case dbNetObj: {
return ((db_id << DBIDTAG_WIDTH) | DBNET_ID);
} break;
case dbModITermObj: {
return ((db_id << DBIDTAG_WIDTH) | DBMODITERM_ID);
} break;Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
…ROAD into debug_or_runtime_fix Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
Signed-off-by: dsengupta0628 <dsengupta@precisioninno.com>
This reverts commit 3db5ed3 for getMTerm speedup as we have better option now.
Signed-off-by: Matt Liberty <mliberty@precisioninno.com>
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 21 days if no further activity occurs. Remove the |
github-actions
Bot
added
the
Stale
4 participants
changes in dbNetwork