Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GC stress log and analysis #108655

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Conversation

markples
Copy link
Member

@markples markples commented Oct 8, 2024

Summary: This includes multiple fixes to reenable the viewing of GC stress log events (from GC dprintf) in StressLogAnalyzer. It makes a small subset of the events available in all _DEBUG builds, which provides a build check in CI without overwhelming the stress log or runtime. This includes an audit of dprintf(1, ...) sites. More detailed GC stress logs can be enabled by changing the sources (as before).

Fixes to StressLogAnalyzer are least-effort because it is intended to be replaced by a C# version in #104999.

Details:

  • gcDetailedStartMsg
    • Split gcDetailedStartMsg to fit under the 16-argument limit for the stress log but keep all arguments for SIMPLE_DPRINTF
    • Cleanup
  • StressLogAnalyzer
    • Handle 16-argument messages
    • Look for a prefix for IS_GCSTART
    • Avoid buffer over-reads
    • Avoid crash on %5s
  • TRACE_GC
    • Enable TRACE_GC in _DEBUG builds
    • Change default behavior to not (re)use upper bits of LogFacility. Leave the mechanism for private builds.
    • Audit dprintf(1) sites and change frequent ones to other values

Includes #108089 until it is merged
Fixes #108628

src/coreclr/gc/gc.h Outdated Show resolved Hide resolved
@@ -1299,6 +1295,7 @@ int ProcessStressLog(void* baseAddress, int argc, char* argv[])
memset(&mapImageToStringId, 0, sizeof(mapImageToStringId));
memset(s_interestingStringFilter, 0, sizeof(s_interestingStringFilter));
memset(s_interestingStringMatchMode, 0, sizeof(s_interestingStringMatchMode));
s_interestingStringMatchMode[IS_GCSTART] = true;
Copy link
Member Author

@markples markples Oct 8, 2024

Choose a reason for hiding this comment

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

This is an unfortunate detail to have here, but I don't think we want to generalize the InterestingStrings table to have a prefix bool.

Copy link
Member

Choose a reason for hiding this comment

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

This will be a little easier to do in the managed implementation I think.

src/coreclr/gc/gcpriv.h Outdated Show resolved Hide resolved
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
@markples markples force-pushed the stresslog-gcstart branch 2 times, most recently from df9e20d to 018d47c Compare October 9, 2024 23:07
@markples
Copy link
Member Author

"ILC: error IL1013: Error processing 'name'" is #108031

@markples markples changed the title Fix GCStart message in StressLog and StressLogAnalyzer Fix GC stress log and analysis Oct 10, 2024
@markples markples marked this pull request as ready for review October 10, 2024 18:16
@markples markples mentioned this pull request Oct 10, 2024
// TRACE_GC has two sub-modes: the standard VM stress log mechanism and
// SIMPLE_DPRINTF, which is text output. By default, we enable TRACE_GC (not
// SIMPLE_DPRINTF) for debug/checked builds so that we can catch build breaks.
// HOST_64BIT is required because logging dprintf to the stress log us only
Copy link
Member Author

Choose a reason for hiding this comment

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

is

// time, which doesn't match usual stress log usage.
//
// In practice, dprintf(1) and LL_INFO10 (which has the value 4) have been used
// similarly on log messages. A dprintf(1) is generally called about once per
Copy link
Member Author

Choose a reason for hiding this comment

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

few

Copy link
Member

@Maoni0 Maoni0 left a comment

Choose a reason for hiding this comment

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

other than the few minor changes you took notes of, the rest LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable TRACE_GC validation build once the build has been fixed
3 participants