-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Eliminate most Windows IO usages in superpmi #121367
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: main
Are you sure you want to change the base?
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
f8cb342 to
e0ea225
Compare
| #include "jitmetadatalist.h" | ||
|
|
||
| fw.Print("\n"); | ||
| fs << std::endl; |
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.
| fs << std::endl; | |
| fs << "\n"; |
std::endl will flush the output stream. We do not want to do that for every context we are processing.
Can you please validate the performance of these changes when running superpmi with the -details argument, especially on slower storage mediums?
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.
Well an error occurs when doing the test with changed executable. I'm going to debug it first.
Data for main with release exe, testing the 72MB mch generated by tests\JIT\superpmi\superpmicollect:
NVMe:925ms HDD:945ms
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.
PR using "\n": NVMe:1050ms HDD:1060ms
Seems that flushing won't be a problem, but formatting becomes less efficiency.
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.
Removing fstream usage reverts the performance back to 945ms.
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.
@jakobbotsch is it acceptable now? Any other concerns?
Continuation of #116422. Contains most usages except I/O redirection for new process.
Combined with little refactors for reading file contents.