Skip to content

GC events changes - #1462

Merged
brianrob merged 1 commit into
microsoft:mainfrom
Maoni0:perheaphist
Jul 22, 2021
Merged

GC events changes#1462
brianrob merged 1 commit into
microsoft:mainfrom
Maoni0:perheaphist

Conversation

@Maoni0

@Maoni0 Maoni0 commented Jul 19, 2021

Copy link
Copy Markdown
Contributor
  1. interpret more fields from the perheap hist event;
  2. add more types for the mark event for .net 6;

@Maoni0

Maoni0 commented Jul 19, 2021

Copy link
Copy Markdown
Contributor Author

this runtime change requires this perfview change otherwise you'll get an exception for the GCStats view.

@Maoni0

Maoni0 commented Jul 19, 2021

Copy link
Copy Markdown
Contributor Author

@dotnet/gc PTAL

@cshung cshung left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@Maoni0

Maoni0 commented Jul 20, 2021

Copy link
Copy Markdown
Contributor Author

@brianrob my runtime changes would require perfview users to get a new version (that includes this change) to work with .NET 6 traces in order to see the GCStats view, are you ok with it?

@brianrob

Copy link
Copy Markdown
Member

Is there a way to detect and report this to the user? Presumably the exception could just do that. Ideally, we make things just work and you don't get the new functionality, but I'm assuming that there was a reason to break compat at the event level?

@Maoni0

Maoni0 commented Jul 21, 2021

Copy link
Copy Markdown
Contributor Author

Is there a way to detect and report this to the user?

without code changes in perfview, all you see is an exception with a callstack that parses the GCMarkWithType event when you try to open the GCStats view. not sure if you meant "Is there a way to detect and report this to the user" in perfview (it would require changes to do that and that would require folks to get a new version anyway). if this is a serious compatibility concern I could bump up the version of the event so without an updated perfview you won't be able to interpret this event for 6.0 traces but will be able to for previous runtime versions

@brianrob

Copy link
Copy Markdown
Member

From PerfView's perspective, probably not a huge deal. For the runtime, though, we document the events, and so I wonder if we should increment the version just to be sure that those who parse the events themselves have this info. At the same time, I don't see a change to the parser - are the event fields staying the same, but interpretation changing?

Also, will .NET 5 and below (including Framework) continue functioning? From the code change, it looks like yes, since I don't see any changes to the event payloads themselves, but want to make sure.

@Maoni0

Maoni0 commented Jul 21, 2021

Copy link
Copy Markdown
Contributor Author

all previous versions of runtime should still work. I added more values to an enum for a field of that event so the array I allocated in perfview (more preciously TraceEvent) to store info for this field is not big enough now (so I'm adding a bigger array now). as long as the field doesn't use the new enum value it should work fine.

@brianrob
brianrob merged commit b8afc73 into microsoft:main Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants