perfview
perfview copied to clipboard
Remove unused code to attach a CLR debugger to a heap dump
It turns out the change in #252 was the result of a misleading message in the console. In reality, due to the change to use clrmd at some point, the attached process is not actually used during extraction of a GC heap from a process dump.
:memo: This change leads to a massive cascade of dead code within heapdump.exe. Since some of that code is technically publicly accessible right now (i.e. could be called by some other library even though we don't ourselves), I'm going to file that as a separate pull request.
Codecov Report
Merging #280 into master will increase coverage by
0.12%
. The diff coverage isn/a
.
@@ Coverage Diff @@
## master #280 +/- ##
==========================================
+ Coverage 17.44% 17.56% +0.12%
==========================================
Files 213 213
Lines 123790 123790
Branches 11971 11971
==========================================
+ Hits 21590 21745 +155
+ Misses 101189 101186 -3
+ Partials 1011 859 -152
Flag | Coverage Δ | |
---|---|---|
#2017 | 17.56% <ø> (+0.12%) |
:arrow_up: |
#Debug | 17.56% <ø> (+0.12%) |
:arrow_up: |
#Release | ? |
Impacted Files | Coverage Δ | |
---|---|---|
src/PerfView/StackViewer/PerfDataGrid.xaml.cs | 34.02% <0%> (ø) |
:arrow_up: |
...c/TraceEvent/Parsers/ClrPrivateTraceEventParser.cs | 19.58% <0%> (+0.03%) |
:arrow_up: |
src/TraceEvent/TraceEvent.cs | 63.34% <0%> (+0.1%) |
:arrow_up: |
src/TraceEvent/DynamicTraceEventParser.cs | 67.49% <0%> (+0.15%) |
:arrow_up: |
src/TraceEvent/TraceLog.cs | 61.25% <0%> (+0.2%) |
:arrow_up: |
src/TraceEvent/Parsers/TplTraceEventParser.cs | 49.65% <0%> (+0.22%) |
:arrow_up: |
src/TraceEvent/ETWTraceEventSource.cs | 47.34% <0%> (+0.23%) |
:arrow_up: |
src/PerfView/StackViewer/CallTreeView.cs | 47.94% <0%> (+0.25%) |
:arrow_up: |
src/TraceEvent/Stacks/FilterStacks.cs | 50.71% <0%> (+0.31%) |
:arrow_up: |
...w/OtherSources/Linux/LinuxPerfScriptEventParser.cs | 77.96% <0%> (+0.34%) |
:arrow_up: |
... and 2 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update ed9cc8d...4378568. Read the comment docs.
I see that we we have been relying on CLRMD for a while now and thus a bunch of code is now dead.
This change could go further (in particular the logic for ICorDebugProcess debugProcess is no longer needed).
I would prefer that all disruptive changes (like that) to eliminate dead / pointless code be done 'together in a single PR as much as possible (can be left as multiple commits).
I want this so that if for whatever reason we want to bring this code back it is should make it easier.
Don't worry about public APIs for this file. The public APIs here are either not used outside perfView are we are willing to break them.
So I would like the ICorDebugProcess debugProcess logic removed, and anything else you think is dead as part of this PR.
The 'Debugger' directory is a clone from another code base (I believe it is mdbg). which we have never had to modify. I am completely OK with the idea of removing whole files that are not needed from this clone. It is not as clear whether it is a good idea to modify these files (even if only to prune) since it really does 'fork' the code base (it becomes hard to know that we can simply 'update' this code by replacement).
If you want a quick merge, we should just remove the full files that we can. Otherwise some thought will be required...
@vancem I reverted all the changes from the last commit except the two small changes which enabled the removal of all the other whole files.
If you want a quick merge, we should just remove the full files that we can.
To be fair, that's why I submitted the PR in the form I did originally. 😉
Could you undo the changes to Debugger.cs and NativeImports.cs since these changes to the file in the Debugger directory that I would like to avoid.
We should also get rid of the code in GCHeapDumper.cs where ICorDebugProcess is passed around (since it is no longer used and can only be confusing). This is the change that I wanted in 'one go' so that if we wanted undo it we could also do it in 'one go'.
@vancem I can't undo the changes in Debugger.cs/NativeImports.cs and remove the files I removed. We can either take neither (my original PR) or both (current PR). Let me know your preference on this.
I'll look at the ICorDebugProcess change now.
@vancem The use of ICorDebugProcess
seems to still be relevant for extracting a GC heap from a currently running process.
Updated after repository rewrite