Skip to content

Conversation

@yowl
Copy link
Contributor

@yowl yowl commented Sep 11, 2023

This PR follows #90847 and removes some more code when FEATURE_EVENT_TRACE is not defined. Surfaced as part of dotnet/runtimelab#2396

Not sure about the gcrhenv.cpp change. Maybe I should be removing all the DiagGC* methods and #ifdefing their usages ?

cc @EgorBo

Thanks

@ghost ghost added area-GC-coreclr community-contribution Indicates that the PR has been added by a community member labels Sep 11, 2023
@ghost
Copy link

ghost commented Sep 11, 2023

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

Issue Details

This PR follows #90847 and removes some more code when FEATURE_EVENT_TRACE is not defined. Surfaced as part of dotnet/runtimelab#2396

Not sure about the gcrhenv.cpp change. Maybe I should be removing all the DiagGC* methods and #ifdefing their usages ?

cc @EgorBo

Thanks

Author: yowl
Assignees: -
Labels:

area-GC-coreclr

Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Sep 11, 2023

LGTM, but leaving for @dotnet/gc to approve/merge

@Maoni0
Copy link
Member

Maoni0 commented Sep 11, 2023

Maybe I should be removing all the DiagGC* methods

you should not remove the methods as GC is still expecting to call them.

for the changed related to the FEATURE_EVENT_TRACE ifdef checks in gc.cpp, why are only some removed but not all?

@EgorBo
Copy link
Member

EgorBo commented Sep 11, 2023

The one in gc_heap::update_ro_segment is just my fault - I used use_frozen_segments_p in an assert and that variable is only available when FEATURE_EVENT_TRACE is defined.

We can also just remove the assert - it doesn't add a lot of value

@yowl
Copy link
Contributor Author

yowl commented Sep 11, 2023

We can also just remove the assert - it doesn't add a lot of value

I have no preference here.

@yowl yowl closed this Sep 25, 2023
@yowl yowl deleted the no-event-trace branch September 25, 2023 21:26
@yowl yowl restored the no-event-trace branch September 27, 2023 17:57
@yowl yowl reopened this Sep 27, 2023
@yowl
Copy link
Contributor Author

yowl commented Sep 27, 2023

Sorry I closed this by mistake.

for the changed related to the FEATURE_EVENT_TRACE ifdef checks in gc.cpp, why are only some removed but not all?

I've gone ahead and just removed the assert so gc.cpp only has that as a change. @Maoni0 is there anything else I can answer or change here? Thanks.

Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

LGTM, but leaving for @dotnet/gc to review/merge

@mangod9
Copy link
Member

mangod9 commented Oct 23, 2023

@cshung, can you please review and merge?

@cshung cshung merged commit 00f1e0c into dotnet:main Oct 23, 2023
@TheSpydog
Copy link
Contributor

Is this fix going to be merged into a .NET 8.x release? For our game console ports of NativeAOT, we do not enable FEATURE_EVENT_TRACE, so we’re running into compile errors when upgrading our runtime support to .NET 8.

@jkotas
Copy link
Member

jkotas commented Nov 14, 2023

/backport to release/8.0-staging

@github-actions
Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/6867071751

@jkotas
Copy link
Member

jkotas commented Nov 14, 2023

We will take it for consideration. We do not typically backport fixes for custom configuration like this one, but I think we can make an exception for this one given that it is a regression from .NET 7, it is shortly after release and the fix is very low risk.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-GC-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants