microsoft / perfview

PerfView is a CPU and memory performance-analysis tool
http://channel9.msdn.com/Series/PerfView-Tutorial
MIT License
4.14k stars 707 forks source link

ThreadID is uint64 in nettrace v4, but handled as int in EventPipeEventSource. #1446

Open lateralusX opened 3 years ago

lateralusX commented 3 years ago

There is currently a downcast of ThreadID read from nettrace v4 into a int. On platforms using pointers as thread id's, there is a risk we will get negative thread id's showing up in SpeedScope and Perfview. It doesn't seem to break anything, but looks "wrong" when displayed and could in theory cause collisions on 64-bit systems (below is from a 32-bit Arm Android trace):

image

What we probably need to do is changing ThreadID to a long (unlong to match nettrace v4) from here, https://github.com/microsoft/perfview/blob/8a34d2d64bc958902b2fa8ea5799437df57d8de2/src/TraceEvent/EventPipe/EventPipeEventSource.cs#L1485 and all the way up the parsing logic.

lateralusX commented 3 years ago

//CC @brianrob @josalem

lateralusX commented 3 years ago

Took a quick look at this and doing it thoroughly will be a very invasive change that probably will break a lot of scenarios, there are hundreds of assumptions across the repro that thread id is of type int, and thread id's might be emitted not just into the record header but might also be part of specific event payloads, and in order for all to work we would need to go through all of this. Most of tools also depend on ETW structs that also handles thread id as int and since it is part of sequential structs, we would need to extend it with an additional 64-bit thread id. If we are going to do this we might need to start with some specific scenario where we have full overview of all usage of thread id, bringing it into full perfview repro seems like a bigger task with big regression risks.

brianrob commented 3 years ago

Yup, unfortunately, TraceEvent uses integers for thread IDs because it was written when it only handled Windows traces, and thread IDs on Windows are 4 bytes wide. Agreed this would have fairly large ramifications.