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.

Sign up for GitHub

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

Jump to bottom

NoGCRegion Callback #82045

Merged
merged 1 commit into from
May 21, 2023
Merged

NoGCRegion Callback #82045

merged 1 commit into from
May 21, 2023

Conversation

cshung
Copy link
Member

@cshung cshung commented Feb 13, 2023 •
edited
Loading

This PR implements the RegisterNoGCCallback API proposal.

Fixes #66039

@cshung cshung marked this pull request as draft February 13, 2023 19:29
@ghost ghost assigned cshung Feb 13, 2023
@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Feb 13, 2023

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

Issue Details

WIP - testing only

Author: cshung
Assignees: cshung
Labels:

area-GC-coreclr, new-api-needs-documentation
Milestone: -

@build-analysis build-analysis bot mentioned this pull request Feb 13, 2023
Open
5 tasks
@cshung cshung force-pushed the public/no-gc-region-callback branch from 58de138 to e647be2 Compare February 15, 2023 20:55
@cshung cshung force-pushed the public/no-gc-region-callback branch 2 times, most recently from 3e1c057 to 6048304 Compare February 27, 2023 21:18
@cshung cshung marked this pull request as ready for review February 28, 2023 06:48
Maoni0
Maoni0 reviewed Mar 1, 2023
View reviewed changes
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
Maoni0
Maoni0 reviewed Mar 1, 2023
View reviewed changes
src/coreclr/gc/gcinterface.h Outdated Show resolved Hide resolved
@cshung cshung force-pushed the public/no-gc-region-callback branch 2 times, most recently from 894f573 to dd909fa Compare March 2, 2023 00:38
This was referenced Mar 2, 2023
Closed
Closed
Open
Closed
@runfoapp runfoapp bot mentioned this pull request Mar 2, 2023
Closed
@cshung cshung force-pushed the public/no-gc-region-callback branch from dd909fa to cbdd477 Compare March 2, 2023 18:49
PeterSolMS
PeterSolMS reviewed Mar 3, 2023
View reviewed changes
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
jkotas
jkotas reviewed May 8, 2023
View reviewed changes
src/coreclr/gc/gcimpl.h Outdated Show resolved Hide resolved
@build-analysis build-analysis bot mentioned this pull request May 9, 2023
Closed
@build-analysis build-analysis bot mentioned this pull request May 11, 2023
Closed
@jkotas
Copy link
Member

jkotas commented May 11, 2023

If the finalizer thread got the work, but then sleep for indefinite amount of time before it actually does the work,

What about if the thread is rescheduled right after we check the cookie, but before actually doing the work?

It seems that the cookie is reducing probability of the race condition, but it is not actually reliably preventing the race condition.

jkotas
jkotas reviewed May 12, 2023
View reviewed changes
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
@jkotas
Copy link
Member

jkotas commented May 12, 2023

VM side - LGTM! @Maoni0 should sign-off on the GC side.

Maoni0
Maoni0 reviewed May 16, 2023
View reviewed changes
src/coreclr/gc/gc.cpp Outdated Show resolved Hide resolved
Maoni0
Maoni0 approved these changes May 16, 2023
View reviewed changes
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 couple of comments above, this LGTM!

@cshung cshung force-pushed the public/no-gc-region-callback branch from e557c26 to 9f1ca40 Compare May 20, 2023 22:27
@cshung @jkotas
NoGCRegion Callback
0ecb53f 
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@cshung cshung force-pushed the public/no-gc-region-callback branch from 9f1ca40 to 0ecb53f Compare May 20, 2023 23:59
@cshung cshung merged commit 6a02ab2 into dotnet:main May 21, 2023
@cshung cshung deleted the public/no-gc-region-callback branch May 21, 2023 01:46
@build-analysis build-analysis bot mentioned this pull request May 21, 2023
Closed
3 tasks
@ghost ghost locked as resolved and limited conversation to collaborators Jun 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@WenceyWang WenceyWang WenceyWang left review comments

@PeterSolMS PeterSolMS PeterSolMS left review comments

@AaronRobinsonMSFT AaronRobinsonMSFT AaronRobinsonMSFT left review comments

@mrsharm mrsharm mrsharm left review comments

@jkotas jkotas jkotas approved these changes

@Maoni0 Maoni0 Maoni0 approved these changes

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

@marek-safar marek-safar Awaiting requested review from marek-safar marek-safar is a code owner

Assignees

@cshung cshung

Labels
area-GC-coreclr new-api-needs-documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[API Proposal]: NoGC callback
7 participants
@cshung @Maoni0 @jkotas @WenceyWang @PeterSolMS @AaronRobinsonMSFT @mrsharm