Skip to content

Conversation

@mikem8361
Copy link
Contributor

@mikem8361 mikem8361 commented Oct 12, 2021

Customer Impact

This is a VS4Mac show stopper. The performance (4 min or so) of taking a core dump when VS4Mac crashes or hangs is unacceptable.

Details

Backport of #60205

Move createdump's DAC enumerate memory region phase after the crash report json file generation is finished. This is so the crash report json is written and available before the core dump which VS4Mac has requested.

Since on both Linux and MacOS all the RW regions have been already added by createdump itself for heap dumps, then the sometimes slow (4 minutes for VS4Mac) heap enum memory region is changed to the faster normal one. It adds necessary DAC globals, etc. without the costly assembly, module, class, type runtime data structure enumeration.

This fast heap dumps is opt'ed in with COMPlus_DbgEnableFastHeapDumps=1 env var to mitigate the risk of something missing from these dumps. With this flag, dump generation is in the seconds now. VS4Mac will set this env var.

Testing

Tested creating a crash report/core dump against VS4Mac process manually. Ran all the SOS tests on MacOS and Linux against this change. The VS4Mac team has signed off on this change.

Risk

Low since there is an opt-in env var that enables the most risk part.

This is a VS4Mac show stopper. The performance (4 min or so) of taking a core dump
when VS4Mac crashes or hangs is unacceptable.

Backport of dotnet#60205

Refactor the DAC enumerate memory region phase out of gather crash info

This is so the crash report json is written and available before the core
dump which for VS4Mac currently takes 4 minutes.

Since on both Linux and MacOS all the RW regions have been already added
by createdump itself for heap dumps, then the sometimes slow (4 minutes for
VS4Mac) heap enum memory region is changed to the faster normal one. It adds
necessary DAC globals, etc. without the costly assembly, module, class, type
runtime data structure enumeration.

This fast heap dumps is opt'ed in with COMPlus_DbgEnableFastHeapDumps=1 env var to mitigate the
risk of something missing from these dumps.

Tested creating a crash report/core dump against VS4Mac process. Ran all the SOS tests on MacOS and Linux against this change.

Low since there is an opt-in env var that enables the most risk part.
@mikem8361 mikem8361 added the Servicing-consider Issue for next servicing release review label Oct 12, 2021
@mikem8361 mikem8361 added this to the 6.0.0 milestone Oct 12, 2021
@mikem8361 mikem8361 requested review from hoyosjs and janvorli October 12, 2021 22:22
@mikem8361 mikem8361 self-assigned this Oct 12, 2021
@ghost
Copy link

ghost commented Oct 12, 2021

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

Customer Impact

This is a VS4Mac show stopper. The performance (4 min or so) of taking a core dump when VS4Mac crashes or hangs is unacceptable.

Details

Backport of #60205

Refactor the DAC enumerate memory region phase out of gather crash info

This is so the crash report json is written and available before the core dump which for VS4Mac currently takes 4 minutes.

Since on both Linux and MacOS all the RW regions have been already added by createdump itself for heap dumps, then the sometimes slow (4 minutes for VS4Mac) heap enum memory region is changed to the faster normal one. It adds necessary DAC globals, etc. without the costly assembly, module, class, type runtime data structure enumeration.

This fast heap dumps is opt'ed in with COMPlus_DbgEnableFastHeapDumps=1 env var to mitigate the risk of something missing from these dumps.

Testing

Tested creating a crash report/core dump against VS4Mac process. Ran all the SOS tests on MacOS and Linux against this change.

Risk

Low since there is an opt-in env var that enables the most risk part.

Author: mikem8361
Assignees: mikem8361
Labels:

Servicing-consider, area-Diagnostics-coreclr

Milestone: 6.0.0

@mikem8361
Copy link
Contributor Author

@cshung could you make sure there isn't anything needed for this 6.0 backport for the GC change you made in PR #60205. It doesn't look like 6.0 needed your change.

@cshung
Copy link
Contributor

cshung commented Oct 12, 2021

@cshung could you make sure there isn't anything needed for this 6.0 backport for the GC change you made in PR #60205. It doesn't look like 6.0 needed your change.

I just checked and I confirm my change in PR #60205 is unnecessary for 6.0

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Please get a few code reviews and then we can take for consideration in .NET 6. Given we are really close to GA, please get a few more code reviewers.

@mikem8361 mikem8361 requested review from jkotas and noahfalk October 12, 2021 23:05
@sgmunn
Copy link

sgmunn commented Oct 14, 2021

I was able to test this with vsmac by, in addition to signing, disabling SIP debug restrictions. The dump was created within, maybe, 10seconds and our crash reporter found the files and processed them. @kdubau and I quickly looked at the dumps and they appear to be what we expect. We believe we can sign off on this for vsmac.

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Oct 14, 2021
@Anipik Anipik merged commit 892555a into dotnet:release/6.0 Oct 14, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants