search

google / UIforETW

User interface for recording and managing ETW traces
https://randomascii.wordpress.com/2015/04/14/uiforetw-windows-performance-made-easier/
Apache License 2.0
1.55k stars 201 forks source link

add CLR runtime tracing support #74

Closed MikeMarcin closed 8 years ago

MikeMarcin commented 8 years ago

Add option to record symbols for ngen CLR dlls.

Allows me to see symbols for WindowsBase.ni.dll and PresentationFramework.ni.dll.

randomascii commented 8 years ago

This looks good and looks like it will be a valuable addition. Do you have any information on what this does to trace sizes? On my one test it seemed to create a huge trace with lots of events lost.

The change description needs a title line followed by a blank line. git convention says that the title line should be no more than about 50 characters: http://chris.beams.io/posts/git-commit/

The mixture of present and past tense made it hard to parse what this actually does. For me these changes read more clearly (even if you are still having these cpu spikes):

< I'm getting cpu spikes

I was getting cpu spikes < It traces down It traced down

I think it's worth adding a sentence saying what this change actually does. i.e.; state explicitly that this gives symbol names to managed code call stacks. It could be as simple as "with this change I now get symbols for WindowsBase.ni.dll and PresentationFrammwork.ni.dll." If you have any information about what this does and doesn't capture (i.e.; what CLR rundown is supposed to add) then that would be helpful.

Spelling error in PresentationFrammwork.ni.dll in the comment.

randomascii commented 8 years ago

Also relevant:

https://github.com/google/UIforETW/issues/48 https://github.com/google/UIforETW/pull/51

MikeMarcin commented 8 years ago

Looks like #51 adds the same support and also tries to add the Rundown Provider. I think the Rundown should be a separate option as the documentation says:

The rundown provider must be turned on for certain special-purpose uses. However, for a majority of users, the runtime provider should suffice.

Wish I'd known about #51 before I did this could've saved me some time. My work is a subset of that PR so feel free to drop this.

randomascii commented 8 years ago

I figured the 'Base' was probably unneeded - good to get that cleaned up.

51 never got finished and it sounds like a subset might be better anyway. I'm happy to land your change - I'm sure I'll use it sometimes. Can you squash it to a single commit?

MikeMarcin commented 8 years ago

After fighting with git for a bit I think I've managed to squash it.

randomascii commented 8 years ago

Hmmm - I merged this but github isn't showing it as merged. And due to the questionable timing of my other change the history didn't end up as clean as I'd hoped. Oh well - it's in there! Thanks!

MagicAndre1981 commented 8 years ago

the rundown provider is required to capture _DC events, so data that occurred before the trace was started. Vance explained it here: https://channel9.msdn.com/Shows/Defrag-Tools/Defrag-Tools-116-PerfView-Part-4#time=19m10s