-
Notifications
You must be signed in to change notification settings - Fork 598
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
Added long running commands runtime to log files. #5260
base: master
Are you sure you want to change the base?
Changes from 3 commits
b4746e1
3d2793c
139a866
fe02c84
1feca9a
d38cfde
a3adf9f
7757589
f86f009
2fba1c3
e18535d
d101470
fd70ec0
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 |
---|---|---|
|
@@ -51,6 +51,7 @@ | |
#include "sta/Search.hh" | ||
#include "sta/SearchPred.hh" | ||
#include "sta/Units.hh" | ||
#include "utl/timer.h" | ||
|
||
namespace rsz { | ||
|
||
|
@@ -93,6 +94,7 @@ void RepairDesign::repairDesign(double max_wire_length, | |
bool verbose) | ||
{ | ||
init(); | ||
utl::ScopedStatistics stat(logger_, "repair_design"); | ||
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. Move to Resizer::repairDesign. This API might be called internally. |
||
int repaired_net_count, slew_violations, cap_violations; | ||
int fanout_violations, length_violations; | ||
repairDesign(max_wire_length, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,7 @@ | |
#include "sta/TimingArc.hh" | ||
#include "sta/Units.hh" | ||
#include "utl/Logger.h" | ||
#include "utl/timer.h" | ||
|
||
namespace rsz { | ||
|
||
|
@@ -93,6 +94,7 @@ void RepairHold::repairHold( | |
const bool verbose) | ||
{ | ||
init(); | ||
utl::ScopedStatistics stat(logger_, "repair_timing"); | ||
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. Move to Resizer::repairHold |
||
sta_->checkSlewLimitPreamble(); | ||
sta_->checkCapacitanceLimitPreamble(); | ||
LibertyCell* buffer_cell = findHoldBuffer(); | ||
|
@@ -132,6 +134,7 @@ void RepairHold::repairHold(const Pin* end_pin, | |
const int max_passes) | ||
{ | ||
init(); | ||
utl::ScopedStatistics stat(logger_, "repair_timing"); | ||
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. Move to Resizer::repairHold |
||
sta_->checkSlewLimitPreamble(); | ||
sta_->checkCapacitanceLimitPreamble(); | ||
LibertyCell* buffer_cell = findHoldBuffer(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,8 +33,13 @@ | |
// | ||
/////////////////////////////////////////////////////////////////////////////// | ||
|
||
#define _GNU_SOURCE | ||
annapetrosyan26 marked this conversation as resolved.
Show resolved
Hide resolved
annapetrosyan26 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
#include "utl/timer.h" | ||
|
||
#include <sys/resource.h> | ||
|
||
#include <fstream> | ||
|
||
namespace utl { | ||
|
||
void Timer::reset() | ||
|
@@ -74,4 +79,70 @@ DebugScopedTimer::~DebugScopedTimer() | |
debugPrint(logger_, tool_, group_.c_str(), level_, msg_, *this); | ||
} | ||
|
||
ScopedStatistics::ScopedStatistics(utl::Logger* logger, std::string msg) | ||
: Timer(), | ||
msg_(msg), | ||
annapetrosyan26 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
start_rsz_(getStartRSZ()), | ||
start_vsz_(getStartVSZ()), | ||
logger_(logger) | ||
{ | ||
} | ||
|
||
size_t ScopedStatistics::getMemoryUsage(const char* tag) | ||
{ | ||
FILE* file = fopen("/proc/self/status", "r"); | ||
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. procfs doesn't exist on OSX unfortunately. src/drt/src/db/infra/frTime_helper.cpp attempts to handle OSX |
||
if (file == NULL) { | ||
annapetrosyan26 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
perror("Failed to open /proc/self/status"); | ||
exit(EXIT_FAILURE); | ||
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. This is too harsh a form of error handling. Issuing a logger warning is sufficient. |
||
} | ||
|
||
size_t result = (size_t) -1; | ||
char line[128]; | ||
size_t tagLength = strlen(tag); | ||
|
||
while (fgets(line, sizeof(line), file) != NULL) { | ||
annapetrosyan26 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (strncmp(line, tag, tagLength) == 0) { | ||
const char* p = line; | ||
while (*p < '0' || *p > '9') | ||
p++; | ||
annapetrosyan26 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
result = (size_t) atoi(p); | ||
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. if this input was human made, I would recommend more error handling, like checking that the number is a valid decimal integer and not something like 00031243, but this is well defined machine generated input, an API to the OS, so I think this is fine as it is. |
||
break; | ||
} | ||
} | ||
|
||
fclose(file); | ||
if (result == (size_t) -1) { | ||
fprintf(stderr, "%s not found in /proc/self/status\n", tag); | ||
exit(EXIT_FAILURE); | ||
} | ||
|
||
return result / 1024; | ||
} | ||
|
||
size_t ScopedStatistics::getStartVSZ() | ||
{ | ||
return getMemoryUsage("VmSize:"); | ||
} | ||
|
||
size_t ScopedStatistics::getPeakVSZ() | ||
{ | ||
return getMemoryUsage("VmPeak:"); | ||
} | ||
|
||
size_t ScopedStatistics::getStartRSZ() | ||
{ | ||
return getMemoryUsage("VmRSS:"); | ||
} | ||
|
||
size_t ScopedStatistics::getPeakRSZ() | ||
{ | ||
return getMemoryUsage("VmHWM:"); | ||
} | ||
annapetrosyan26 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
ScopedStatistics::~ScopedStatistics() | ||
{ | ||
logger_->report(msg_ + ": runtime {} seconds, usage: rsz = {} MB, vsz = {} MB, peak: rsz = {} MB, vsz = {} MB", static_cast<int>(Timer::elapsed()), (getPeakRSZ() - start_rsz_), | ||
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. runtime is unclear as it could be elapsed or cpu time. I think using elapsed is clearer. Personally I think peak is sufficient (@oharboe any opinion)? 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. If we don't know the starting memory consumption, we can miss false positives: the problem is not with the algorithm that is reported as too high usage, but something in the setup. 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. This doesn't tell you the starting memory - only the end - start 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. correct. pick a way to present two numbers.... start memory is trivially calculated from peak and usagr, true for any cpmbination of these two numbers. most time usage and peak is what is of interested 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. usage and peak do not give start. The peak may well be in the middle not at the end. 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. Which is what peak should say and why I don't think usage matters much. 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. Agreed. Keep it simple. If there is something wrong(large initial memory allocation outside of scope), then it can be debugged otherwise. I think normal operation, determining required RAM, is more important and other information clouds that... 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. so let's go with the DRT format above (with prefix) 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. From the conversation below it's not clear what the final message should be. Please confirm if it should look like the DRT message you provided. 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. I was thinking the prefix would be about what is being timed, eg
Though the prefix would be up to the client code. |
||
(getPeakVSZ() - start_vsz_), (getPeakRSZ()), (getPeakVSZ())); | ||
} | ||
|
||
} // namespace utl |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Passed: Statistics are logged. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import os | ||
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. hook up to CI? 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. Isn't using 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. What I mean with "CI" is continuous integration, I wanted to know if this test is hooked up to the tests run on pull requests in github. Regarding os vs pathlib, I think 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. This test is included in the regression, so it's in CI. |
||
import subprocess | ||
import re | ||
|
||
def run_regression_script(regression_script_path, argument): | ||
try: | ||
subprocess.run([regression_script_path, argument], check=True) | ||
except subprocess.CalledProcessError as e: | ||
print(f"Error running regression script: {e}") | ||
return False | ||
return True | ||
|
||
def check_log_file(log_path): | ||
pattern = re.compile(r"repair_design: runtime.*seconds.*usage.*rsz.*MB.*vsz.*MB.*peak.*rsz.*vsz.*") | ||
runtime_count = 0 | ||
|
||
with open(log_path, 'r') as log_file: | ||
for line in log_file: | ||
if pattern.search(line): | ||
runtime_count += 1 | ||
if runtime_count == 2: | ||
print("Passed: Statistics are logged.") | ||
exit(0) | ||
else: | ||
print("Statistics are not logged enough times.") | ||
exit(1) | ||
|
||
def main(): | ||
script_dir = os.path.dirname(os.path.abspath(__file__)) | ||
base_dir = os.path.abspath(os.path.join(script_dir, "../../rsz/test")) | ||
log_path = os.path.join(base_dir, "results", "repair_design4-tcl.log") | ||
regression_script_path = os.path.join(base_dir, "regression") | ||
|
||
if not os.path.exists(log_path): | ||
if not run_regression_script(regression_script_path, "repair_design4"): | ||
exit(1) | ||
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. Why is a regression test calling regression again? This will create a conflict with the results of running repair_design4 (especially if run in parallel with this test). It is better to make this test self-contained. |
||
|
||
check_log_file(log_path) | ||
|
||
if __name__ == "__main__": | ||
main() | ||
|
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 should go in Replace::doNesterovPlace to capture initial placement as well.