Skip to content

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.

Sign up for GitHub

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

Open
tommcdon wants to merge 2 commits into
base: main
Choose a base branch
from
+1 −3
Open

Fix perfmap output #122273

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c93c88a
Fix perfmap output
tommcdon Dec 7, 2025
d18f1ca
Use fprintf as it is more performant than write for perfmaps
tommcdon Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/coreclr/vm/perfmap.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,14 +261,16 @@ void PerfMap::WriteLine(SString& line)
EX_TRY
{
// Write the line.

if (fprintf(m_fp, "%s", line.GetUTF8()) != 0)
Copy link
Member

@jkotas jkotas Dec 8, 2025

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?

Copy link
Member Author

@tommcdon tommcdon Dec 8, 2025

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.

Copy link
Member

@huoyaoyuan huoyaoyuan Dec 8, 2025

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 write to accomplish the same task.

Or maybe fwrite to be consistent at stdio level?

int fd = fileno(m_fp);
const char* buf = line.GetUTF8();
size_t len = line.GetCount();
ssize_t written = write(fd, buf, len);
Copy link
Member

@jkotas jkotas Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • FILE does buffering for better perf. Using write sys-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

    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);
    . Also, we can open/close the flle using open/close syscalls and avoid using FILE.

if (written < 0 || (size_t)written != len)
{
// This will cause us to stop writing to the file.
// The file will still remain open until shutdown so that we don't have to take a lock at this level when we touch the file stream.
m_ErrorEncountered = true;
}

}
EX_CATCH{} EX_END_CATCH
}
Expand Down
Loading