-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix perfmap output #122273
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?
Fix perfmap output #122273
Conversation
|
Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag |
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.
Pull request overview
This PR fixes a regression in perfmap output caused by incorrect error checking in fprintf usage. The previous code treated successful writes as errors due to checking if the return value was not equal to zero, when fprintf actually returns the number of characters written (> 0) on success.
Key Changes
- Replaced
fprintf(m_fp, "%s", line.GetUTF8())with a directwrite()system call using the file descriptor - Fixed error checking to properly detect write failures and partial writes
| { | ||
| // Write the line. | ||
|
|
||
| if (fprintf(m_fp, "%s", line.GetUTF8()) != 0) |
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.
What is the problem with fprintf?
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.
The bug was introduced by comparing the output of fprintf to 0. fprintf returns a value < 0 on failure, otherwise the number of bytes written to the stream.
Since we always use a formatting character of "%s", we can use write to accomplish the same task.
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.
It may be valuable to double check the return value handling of each stdio function. Some returns error code and some returns meaningful length.
Since we always use a formatting character of "%s", we can use
writeto accomplish the same task.
Or maybe fwrite to be consistent at stdio level?
|
cc @huoyaoyuan |
src/coreclr/vm/perfmap.cpp
Outdated
| int fd = fileno(m_fp); | ||
| const char* buf = line.GetUTF8(); | ||
| size_t len = line.GetCount(); | ||
| ssize_t written = write(fd, buf, len); |
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.
FILEdoes buffering for better perf. Usingwritesys-call directly is going to be slower. Would it be better to use fwrite to benefit from the buffering?- If we want to use direct sycallls for some reason, the syscalls should be wrapped in EINTR dance like . Also, we can open/close the flle using open/close syscalls and avoid using FILE.
runtime/src/native/libs/System.Native/pal_io.c
Line 1262 in 74cf618
while ((bytesWritten = write(outFd, buffer + offset, (size_t)bytesRead)) < 0 && errno == EINTR);
| // Write the line. | ||
|
|
||
| if (fprintf(m_fp, "%s", line.GetUTF8()) != 0) | ||
| if (fprintf(m_fp, "%s", line.GetUTF8()) != line.GetCount()) |
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.
SString::GetCount is going to convert the string to Unicode and return the number of unicode characters:
runtime/src/coreclr/inc/sstring.inl
Lines 1172 to 1174 in 74cf618
| ConvertToFixed(); | |
| SS_RETURN SizeToCount(GetSize()); |
Fix a recent regression in perfmaps caused by #116203
The fix changes fprintf usage in PerfMap::WriteLine to using