perfview icon indicating copy to clipboard operation
perfview copied to clipboard

Remove unused code to attach a CLR debugger to a heap dump

Open sharwell opened this issue 7 years ago • 8 comments

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.

sharwell avatar Jun 09 '17 13:06 sharwell

Codecov Report

Merging #280 into master will increase coverage by 0.12%. The diff coverage is n/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.

codecov-io avatar Jun 09 '17 13:06 codecov-io

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.

vancem avatar Jun 12 '17 01:06 vancem

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 avatar Jun 13 '17 01:06 vancem

@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. 😉

sharwell avatar Jun 14 '17 02:06 sharwell

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 avatar Jun 14 '17 14:06 vancem

@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.

sharwell avatar Jun 14 '17 14:06 sharwell

@vancem The use of ICorDebugProcess seems to still be relevant for extracting a GC heap from a currently running process.

sharwell avatar Jun 14 '17 14:06 sharwell

Updated after repository rewrite

sharwell avatar Feb 02 '18 12:02 sharwell