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
Add randomized allocation sampling #100356
Add randomized allocation sampling #100356
Changes from 59 commits
3815aa8
6a92fcb
cdbffc3
924afd0
e0ce093
396c05e
91b8b79
65ff391
b25adc6
d939cdd
9b0f7da
8875a4d
f39a732
fbc0b6b
32eb2fa
518b4f7
b596f33
984ed95
1c46a5d
44e914b
9eba36e
b7614fd
cbc5a12
826d8eb
a56a5ee
48dbfa1
7280c13
fd8a1b9
a4aef1a
08ec139
0bbbfa9
cf29e9b
26e8fe0
52d4071
db506a3
8522866
e1f8ece
32f6d31
9e31a6e
f9f5224
97a3bbd
734b773
a6120ac
3d0bcd1
549e7b0
147447a
2b41997
4618b37
918fe0c
e82162d
09eae74
6badfb2
d0e46a4
ef5b819
ff77f7a
77cb736
c40b1a2
a62ea75
07ab88b
d04b655
d565045
b7bd18f
7cbcb43
86f24c6
51d9935
a9ef0e5
dd7e00d
a86a98b
512f011
532f095
4fc9b8d
9174212
541f59f
40f96f3
29c16b5
20c6d51
c5ccad8
39afdf2
b2db19e
e4ef681
abfe6cb
997a293
041e397
e312bf6
ec16fa0
fd382d3
a2e30cc
83611b8
c908fe4
dc02b7c
85625cd
911d5d7
91d58dc
11177d9
ce40d3d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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 enum is used by the GC/EE interface. Changing the size of the enum to
uint64_t
would require revvingGC_INTERFACE_MAJOR_VERSION
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 expect that @Maoni0 would prefer to avoid revving the GC interface major version and do this in some other way.
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.
@noahfalk @brianrob: the current implementation of the provider/keyword/verbosity status update in EtwCallbackCommon casts the 64 bit received keyword into a 32 bit enum for the GC. Unfortunately, the AllocationSamplingKeyword is beyond this range.
I did not realize (thanks for pointing this out @jkotas) that it would require to change the signature of the GC interface to support it.
Any idea?
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.
So far, I'm not seeing something that I would recommend. I can think of at least one way to do this, but I don't even want to say it because I think it could break us into eventual jail. The idea would be to use a bit that is already used on the ETW keyword mask, but not sent to the GC because the GC doesn't currently use it. The risk is that if the GC ever wanted to use said bit, then there would be a problem.
So far, revving the interface seems best to me.
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 we could just do the filtering on the VM side in the GCToCLREventSink no?
runtime/src/coreclr/vm/gctoclreventsink.cpp
Line 163 in f6701e5
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.
Yes, that would be fine. I had been trying to avoid the interface call cost, but I think it will not be a big deal, and certainly more preferrable to changing the interface.
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.
By following the execution path of
GCHeapUtilities::RecordEventStateChange
(for Core and NativeAOT), I think that it should be possible to call a newGCHeapUtilities::AllocationSampledEventState(bool enable)
based on the value of the received keyword. This would be non generic processing like what is done for triggering a full GC.On the GC side, a simple boolean field in
GCEventStatus
would keep track of the state and be exposed like in my current code viaGCEventEnabledAllocationSampled()
.This would also allow enable/disable detection that does not seem possible today (once enabled, a provider/keyword/verbosity stays enabled) and... avoid the virtual call ;^)
Anyway... I've pushed your idea @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.
You may already have this on a list somewhere, but
name
is alwaysnull
(I'm not sure that nativeaot even has the type name data available...)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'm reusing the same code as AllocationTick to get the type name and it is working in the tests for Core. I'll try the same code for NativeAOT when the compilations errors are fixed.
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.
Sounds good. I suspect that nativeaot doesn't even have the type names, so it may just always need to be null, but wanted to call it out.
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.
Sorry @brianrob!
I missed that your point was for NativeAOT only!