Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Report -1 for GC.GetGeneration(nongc_obj) and same for GetObjectGeneration #85017
Report -1 for GC.GetGeneration(nongc_obj) and same for GetObjectGeneration #85017
Changes from 1 commit
0ac29ee
71f63b2
c3754e0
d1268e5
8072970
d57e193
8720b4f
0e422ab
330f22a
d63b652
2135544
b50f36a
2ce1588
fdcfbdd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can return memory range for the given frozen segments here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I thought that the idea was to not provide support for apis with explicit GC prefix/name/suffix for nongc objects (cc @noahfalk)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the plan of record is to avoid treating Non-GC objects as if they are on the GC heap, and that also included not treating the non-GC segments as if they are some type of special GC generation. We could smuggle the range here at the cost of blurring the lines and potentially creating some confusion. If profiler vendors said it would be specifically helpful to them I don't think it bends the rules too badly though. From the limited profiler feedback I've heard so far folks wanted to enumerate the Non-GC region boundaries directly and resolve the pointers themsevles offline.
I'm expecting @davmason to broadcast the changes we are doing to profiler vendors and then depending on their feedback we can make adjustments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning error here makes this more breaking than necessary. Why can't we just return
S_OK
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @noahfalk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning -1 seemed like it was going to be a breaking change no matter what so I figured we may as well make it more obvious rather than try to minimize it and increase risk that tools break more subtly. However we don't yet have feedback from profiler authors one way or another and this behavior is just a best guess on my part. If you think we'd be better off defaulting to keeping S_OK @jkotas I don't have a strong opinion on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Traditionally (in textbook sense) GC generations imply that genX can be collected without collecting generations X+.
-1
makes impression that any level of GC will collect frozen, while in fact it is the opposite - any generation can be collected without touching frozen.That seems to imply that frozen objects logically belong to generation > MAX_GEN.
Perhaps
3
or MAXINT could work better that-1
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks odd to me to return failure error code and still try to return meaningful values. I see that there are other profiler APIs that work like that, so it should be fine to return error code. You can ignore my feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of agree with @VSadov that e.g. INT_MAX value would be safer, e.g. I already had to apply a hack in code because GC/VM usages of
WhichGeneration
participate in comparisons so -1 will trigger a different path than previouslymax_generation
did. Although, this is a part of the plan to make this breaking change more visible so 🤷There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no particular preference between MAXINT and -1. I don't anticipate its going to make a difference myself but I also have no concern changing it. 3 I would not use - in the right context 3 is used as a legitimate generation number and the goal is that nobody should treat this value as if it is a generation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are not that many purposes for
WhichGeneration
other than to answer questions like “can this object be collected before that one?” or “will this object survive GC X?”.Assigning immortal objects to gen
-1
feels like not the most convenient/intuitive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried INT_MAX and there was only one place that I had to change (some NativeAOT test expecting frozen objects to be in Gen2). Also,
GC.Collect(GC.GetGeneration..
is not breaking now as Jan highlighted.For -1 it becomes a little bit more complicated.
So any strong opinions for keeping -1 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
INT_MAX
is the correct value. Even with unforeseen future changes, I do not see anything collecting less often than frozen.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might not work for ReadyToRun?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, disabled the test for crossgen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that this is correct. Did you see the test actually failing with crossgen?
If I am reading the code correctly, this test should work with crossgen just fine. The string literals referenced by R2R code must be allocated on frozen heap. Otherwise, the tiering from R2R to Tier1 would work poorly - the tiering would be stuck with non-frozen string literals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, you're absolutely right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about calling this API to verify that it is a frozen object as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding we sort of ignore these old Frozen* profiler APIs as we re-branded Frozen as NonGC. For the same reason we introduce a new API to traverse frozen objects and ignore existing
EnumModuleFrozenObjects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore is probably fine when we were still designing, but it is really lousy from a testing perspective. It should either supposed to work or it doesn't. If it is supposed to work, and we are not testing it, it would be a test hole. If it is not supposed to work, then we should update the code to make it fails. If we wanted to depreciate old APIs in favor of new ones, we should make that clear in the documentation. Either way, something is missing.