Skip to content

Conversation

@jkotas
Copy link
Member

@jkotas jkotas commented Mar 18, 2021

Customer Impact

Intermittent crash during GC in complex scenarios that use collectible types

Description

It looks like a bug that has been there since forever. The bug just results into occasional small GC inefficiency most of the time. The crash only happens when all of the following is true:

  • The workload uses collectible types
  • The GC hits mark stack overflow (ie the object graph is very deep and complex)
  • The overflowing objects have collectible types
  • The MethodTable of the overflowing objects is allocated at the start of the page and the preceding page is unmapped. (The crash is caused by accesing this unmapped page.)

Regression?

No.

Risk (see taxonomy)

Low. Fix validated by the customer.

Fixes #49684

@ghost ghost added the area-GC-coreclr label Mar 18, 2021
@ghost
Copy link

ghost commented Mar 18, 2021

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

Issue Details

Fixes #49684

Author: jkotas
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@jkotas jkotas mentioned this pull request Mar 18, 2021
@jkotas jkotas requested a review from davidwrighton March 18, 2021 15:24
@jkotas jkotas marked this pull request as ready for review March 18, 2021 15:27
@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Mar 18, 2021
@jeffschwMSFT jeffschwMSFT added this to the 5.0.x milestone Mar 18, 2021
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 cr and we will take for consideration in 5.0.x

Copy link
Member

@davidwrighton davidwrighton left a comment

Choose a reason for hiding this comment

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

Assuming you agree with my comment, this is approved.

CGCDescSeries* cur = map->GetHighestSeries();
ptrdiff_t cnt = (ptrdiff_t)map->GetNumSeries();

if (cnt >= 0)
Copy link
Member

Choose a reason for hiding this comment

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

My analysis indicates the change here to change from > to >= is a small performance optimization. From my analysis cnt == 0 can't actually happen so this is ok, but I want to verify that was the rationale.

Copy link
Member Author

Choose a reason for hiding this comment

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

My primary motivation for this was to make the conditions and flow to be more similar to the go_through_object macros, so that easier to validate that the logic in this method matches it.

You are right that the side-effect is minor perf optimization.

@leecow leecow added the Servicing-approved Approved for servicing release label Mar 23, 2021
@leecow leecow modified the milestones: 5.0.x, 5.0.6 Mar 23, 2021
@jeffschwMSFT jeffschwMSFT removed the Servicing-consider Issue for next servicing release review label Mar 25, 2021
@Anipik Anipik merged commit 6535324 into dotnet:release/5.0 Apr 6, 2021
@jkotas jkotas deleted the release/5.0-issue-49684 branch April 6, 2021 18:01
@ghost ghost locked as resolved and limited conversation to collaborators May 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-GC-coreclr Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants