Skip to content

Commit

Permalink
Partition based on samples instead of traceEvents when importing a sa…
Browse files Browse the repository at this point in the history
…mple-based chrome trace (jlfwong#460)

In the existing code, if `traceEvents` did not have any importable events, the profile would be marked as empty. This was a bug, as the dev Hermes profiles I was testing with had one X event which made the code work. However we do not need to guarantee (and the spec doesn't seem to) any traceEvents being present as long as we have samples and stack frames. 

When I tested a production profile taken from Hermes it did not have any importable events, just a metadata event with a thread name. This PR updates the implementation to iterate over partitioned samples instead of traceEvents so we construct profiles for all thread + process pairs referenced in the samples array.

| Before | After |
| --- | ----- |
| <img width="1454" alt="Screenshot 2024-01-03 at 9 58 56 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/78c098a7-b374-4492-ad13-9ca79afdb40c"> | <img width="1450" alt="Screenshot 2024-01-03 at 9 58 17 AM" src="https://github.com/jlfwong/speedscope/assets/9957046/d2d5e82b-fa3e-43db-bf8a-e8c3b84cd13a"> |
  • Loading branch information
zacharyfmarion authored Jan 3, 2024
1 parent a3c0b15 commit a6d7001
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 34 deletions.
10 changes: 0 additions & 10 deletions sample/profiles/trace-event/simple-with-samples.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,6 @@
"args": {
"name": "mqt_js"
}
},
{
"name": "mqt_js",
"cat": "mqt_js",
"ph": "X",
"dur": 0,
"pid": 7512,
"ts": "11550183666",
"tid": "7695",
"args": {}
}
],
"samples": [
Expand Down
72 changes: 48 additions & 24 deletions src/import/trace-event.ts
Original file line number Diff line number Diff line change
Expand Up @@ -349,14 +349,50 @@ function frameInfoForEvent(
}
}

/**
* Constructs an array mapping pid-tid keys to profile builders. Both the traceEvent[]
* format and the sample + stack frame based object format specify the process and thread
* names based on metadata so we share this logic.
*
* See https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU/preview#heading=h.xqopa5m0e28f
*/
function getProfileNameByPidTid(
function getProfileName(
processName: string | undefined,
threadName: string | undefined,
pid: number,
tid: number,
): string {
if (processName != null && threadName != null) {
return `${processName} (pid ${pid}), ${threadName} (tid ${tid})`
} else if (processName != null) {
return `${processName} (pid ${pid}, tid ${tid})`
} else if (threadName != null) {
return `${threadName} (pid ${pid}, tid ${tid})`
} else {
return `pid ${pid}, tid ${tid}`
}
}

function getProfileNamesFromSamples(
events: TraceEvent[],
partitionedSamples: Map<string, Sample[]>,
): Map<string, string> {
const processNamesByPid = getProcessNamesByPid(events)
const threadNamesByPidTid = getThreadNamesByPidTid(events)

const profileNamesByPidTid = new Map<string, string>()

partitionedSamples.forEach(samples => {
if (samples.length === 0) return

const pid = Number(samples[0].pid)
const tid = Number(samples[0].tid)

const profileKey = pidTidKey(pid, tid)
const processName = processNamesByPid.get(pid)
const threadName = threadNamesByPidTid.get(profileKey)
const profileName = getProfileName(processName, threadName, pid, tid)

profileNamesByPidTid.set(profileKey, profileName)
})

return profileNamesByPidTid
}

function getProfileNamesFromTraceEvents(
events: TraceEvent[],
partitionedTraceEvents: Map<string, TraceEvent[]>,
): Map<string, string> {
Expand All @@ -373,19 +409,9 @@ function getProfileNameByPidTid(
const profileKey = pidTidKey(pid, tid)
const processName = processNamesByPid.get(pid)
const threadName = threadNamesByPidTid.get(profileKey)
const profileName = getProfileName(processName, threadName, pid, tid)

if (processName != null && threadName != null) {
profileNamesByPidTid.set(
profileKey,
`${processName} (pid ${pid}), ${threadName} (tid ${tid})`,
)
} else if (processName != null) {
profileNamesByPidTid.set(profileKey, `${processName} (pid ${pid}, tid ${tid})`)
} else if (threadName != null) {
profileNamesByPidTid.set(profileKey, `${threadName} (pid ${pid}, tid ${tid})`)
} else {
profileNamesByPidTid.set(profileKey, `pid ${pid}, tid ${tid}`)
}
profileNamesByPidTid.set(profileKey, profileName)
})

return profileNamesByPidTid
Expand Down Expand Up @@ -616,7 +642,7 @@ function eventListToProfileGroup(
): ProfileGroup {
const importableEvents = filterIgnoredEventTypes(events)
const partitionedTraceEvents = partitionByPidTid(importableEvents)
const profileNamesByPidTid = getProfileNameByPidTid(events, partitionedTraceEvents)
const profileNamesByPidTid = getProfileNamesFromTraceEvents(events, partitionedTraceEvents)

const profilePairs: [string, Profile][] = []

Expand Down Expand Up @@ -647,10 +673,8 @@ function eventListToProfileGroup(
}

function sampleListToProfileGroup(contents: TraceWithSamples): ProfileGroup {
const importableEvents = filterIgnoredEventTypes(contents.traceEvents)
const partitionedTraceEvents = partitionByPidTid(importableEvents)
const partitionedSamples = partitionByPidTid(contents.samples)
const profileNamesByPidTid = getProfileNameByPidTid(contents.traceEvents, partitionedTraceEvents)
const profileNamesByPidTid = getProfileNamesFromSamples(contents.traceEvents, partitionedSamples)

const profilePairs: [string, Profile][] = []

Expand Down

0 comments on commit a6d7001

Please sign in to comment.