Eliminate most Windows IO usages in superpmi #121367
Conversation
github-actions
bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
huoyaoyuan
force-pushed
the
superpmi/stdio2
branch
from
f8cb342 to
e0ea225
Compare
| #include "jitmetadatalist.h" | ||
|
|
||
| fw.Print("\n"); | ||
| fs << std::endl; |
There was a problem hiding this comment.
| 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.
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.
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.
Removing fstream usage reverts the performance back to 945ms.
There was a problem hiding this comment.
@jakobbotsch is it acceptable now? Any other concerns?
| char* nameOfFile1; | ||
| char* nameOfFile2; | ||
| char* nameOfFile3; | ||
| int indexCount; |
There was a problem hiding this comment.
If we're using a vector now, can we remove the indexCount member?
There was a problem hiding this comment.
It's also used as a sentinel for not using the indices. I haven't looked about it in depth.
There was a problem hiding this comment.
It's passed into MethodContextReader which doesn't make a good lifetime. I tried to use shared_ptr<vector> to pass the list but got really confused. I'd like to limit the refactor and touch types that directly interact with the IO.
| cr->applyRelocs(&rc, coldCodeBlock, coldCodeSize, orig_coldCodeBlock); | ||
| cr->applyRelocs(&rc, roDataBlock, roDataSize, orig_roDataBlock); | ||
|
|
||
| #ifdef USE_MSVCDIS |
There was a problem hiding this comment.
This can be defined in internal builds. See standardpch.h. The @dotnet/jit-contrib should weight in on if this is still used in some scenarios. There are several places with USE_MSVCDIS.
| #include "logging.h" | ||
|
|
||
| bool MCList::processArgAsMCL(char* input, int* count, int** list) | ||
| bool MCList::processArgAsMCL(char* input, std::vector<int>& list) |
There was a problem hiding this comment.
The behavior here is now quite different as this now assumes the list is empty. Please assert that is the case.
Co-authored-by: Aaron R Robinson <arobins@microsoft.com>
| } | ||
|
|
||
| bool WriteArrayToMCL(char* mclFilename, int* arr, int count) | ||
| bool WriteArrayToMCL(char* mclFilename, std::vector<int>& MCL) |
There was a problem hiding this comment.
| bool WriteArrayToMCL(char* mclFilename, std::vector<int>& MCL) | |
| bool WriteArrayToMCL(char* mclFilename, const std::vector<int>& MCL) |
|
Is there any remaining issue for this? |
AaronRobinsonMSFT
left a comment
AaronRobinsonMSFT
left a comment
There was a problem hiding this comment.
I defer to @dotnet/jit-contrib for the official sign off. This seems like a noisy PR with lots of subtle changes that might impact stability. Ideally it would be smaller and more targeted, but I get that may not be feasible. Another set of eyes is needed.
3 participants
Continuation of #116422. Contains most usages except I/O redirection for new process.
Combined with little refactors for reading file contents.