Skip to content

Revise perfjitdump #229

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

Merged
merged 6 commits into from
Nov 26, 2019
Merged

Revise perfjitdump #229

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
05c870e
Revise perfjitdump
sdmaclea Nov 22, 2019
acd8546
Adjust iov_base
sdmaclea Nov 22, 2019
333e6da
Clean up
sdmaclea Nov 23, 2019
867357f
Cleanup
sdmaclea Nov 25, 2019
188d37c
Mark codeIndex volatile
sdmaclea Nov 25, 2019
dbed743
Cleanup
sdmaclea Nov 25, 2019
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
71 changes: 52 additions & 19 deletions src/coreclr/src/pal/src/misc/perfjitdump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <sys/types.h>
#include <sys/stat.h>
#include <sys/syscall.h>
#include <sys/uio.h>
#include <time.h>
#include <unistd.h>
#include <linux/limits.h>
Expand Down Expand Up @@ -135,11 +136,11 @@ struct PerfJitDumpState
codeIndex(0)
{}

bool enabled;
volatile bool enabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use of volatile in C++ always make me uneasy that it's actually hiding something else. I haven't read the code for PerfJitDumpState yet, but what's the reason to qualify enabled as volatile here?

Copy link
Contributor Author

@sdmaclea sdmaclea Nov 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

It is a variable that is expected to be written by another thread during Finish.

Before we lock the mutex, we read enabled. After locking I want to make sure enabled is read again in case Finish disabled it. I marked it as volatile to prevent the optimizer from using the value it had initially read. Given that the locking is a function call it might not be needed, but I don't fully understand the nuances of the C++ optimization rules.

int fd;
void *mmapAddr;
pthread_mutex_t mutex;
uint64_t codeIndex;
volatile uint64_t codeIndex;

int FatalError(bool locked)
{
Expand Down Expand Up @@ -232,11 +233,23 @@ struct PerfJitDumpState

JitCodeLoadRecord record;

size_t bytesRemaining = sizeof(JitCodeLoadRecord) + symbolLen + 1 + codeSize;

record.header.timestamp = GetTimeStampNS();
record.vma = (uint64_t) pCode;
record.code_addr = (uint64_t) pCode;
record.code_size = codeSize;
record.code_index = ++codeIndex;
record.header.total_size = sizeof(JitCodeLoadRecord) + symbolLen + 1 + codeSize;
record.header.total_size = bytesRemaining;

iovec items[] = {
// ToDo insert debugInfo and unwindInfo record items immediately before the JitCodeLoadRecord.
{ &record, sizeof(JitCodeLoadRecord) },
{ (void *)symbol, symbolLen + 1 },
{ pCode, codeSize },
};
size_t itemsCount = sizeof(items) / sizeof(items[0]);

int itemsWritten = 0;

result = pthread_mutex_lock(&mutex);

Expand All @@ -246,36 +259,56 @@ struct PerfJitDumpState
if (!enabled)
goto exit;

// ToDo write debugInfo and unwindInfo immediately before the JitCodeLoadRecord (while lock is held).
// Increment codeIndex while locked
record.code_index = ++codeIndex;

record.header.timestamp = GetTimeStampNS();
do
{
result = writev(fd, items + itemsWritten, itemsCount - itemsWritten);

result = write(fd, &record, sizeof(JitCodeLoadRecord));
if (result == bytesRemaining)
break;

if (result == -1)
return FatalError(true);
if (result == -1)
{
if (errno == EINTR)
continue;

result = write(fd, symbol, symbolLen + 1);
return FatalError(true);
}

if (result == -1)
return FatalError(true);
// Detect unexpected failure cases.
_ASSERTE(bytesRemaining > result);
_ASSERTE(result > 0);

result = write(fd, pCode, codeSize);
// Handle partial write case

if (result == -1)
return FatalError(true);
bytesRemaining -= result;

result = fsync(fd);
do
{
if (result < items[itemsWritten].iov_len)
{
items[itemsWritten].iov_len -= result;
items[itemsWritten].iov_base = (void*)((size_t) items[itemsWritten].iov_base + result);
break;
}
else
{
result -= items[itemsWritten].iov_len;
itemsWritten++;

if (result == -1)
return FatalError(true);
// Detect unexpected failure case.
_ASSERTE(itemsWritten < itemsCount);
}
} while (result > 0);
} while (true);

exit:
result = pthread_mutex_unlock(&mutex);

if (result != 0)
return FatalError(false);

}
return 0;
}
Expand Down