Proposed changes

- Renamed alloc_sampling -> combined_limit
   We were refering to alloc_sampling as the sampling limit but really it is min(sampling_limit, alloc_limit).
   Most of the time alloc_sampling was the AC limit and not a sampling limit. It seemed misleading to
   have 'sampling' in the name when most of the time that isn't what it repreesnts.

- Simplified logic in Alloc()
   Special casing UOH allocations appears unnecessary, performance still looked good without doing that
   Factored all the checks to see if the feature was on inside a IsRandomizedSamplingEnabled() func
   Moved all the fields that are only needed for sampling inside the scope where sampling is enabled
   Adjusted some comments

-Fixed the sampled byte reporting for objects that extend past the AC boundary

- Removed unneeded code changes that were fragments of past code changes no longer being used

- Removed errant whitespace changes in gc code

- consolidated test changes in a folder named "randomizedallocationsampling". We don't need multiple
  folders and I'm trying to consolidate naming this feature as 'randomized' allocation rather than
  'dynamic'

- moved the testing results into a dedicated folder that can be deleted at the end to make it clear what
  stays and what is temporary just for review.

src/coreclr/gc/gc.cpp

@@ -48192,7 +48192,7 @@ HRESULT GCHeap::Initialize()
     /*
      * Allocation requests less than loh_size_threshold will be allocated on the small object heap.
      *
-     * An object cannot span more than one region and regions in small object heap are of the same size - gc_region_size.
+     * An object cannot span more than one region and regions in small object heap are of the same size - gc_region_size. 
      * However, the space available for actual allocations is reduced by the following implementation details -
      *
      * 1.) heap_segment_mem is set to the new pages + sizeof(aligned_plug_and_gap) in make_heap_segment.
@@ -48208,7 +48208,7 @@ HRESULT GCHeap::Initialize()
 #ifdef FEATURE_STRUCTALIGN
     /*
      * The above assumed FEATURE_STRUCTALIGN is not turned on for platforms where USE_REGIONS is supported, otherwise it is possible
-     * that the allocation size is inflated by ComputeMaxStructAlignPad in GCHeap::Alloc and we have to compute an upper bound of that
+     * that the allocation size is inflated by ComputeMaxStructAlignPad in GCHeap::Alloc and we have to compute an upper bound of that 
      * function.
      *
      * Note that ComputeMaxStructAlignPad is defined to be 0 if FEATURE_STRUCTALIGN is turned off.

src/coreclr/gc/gcpriv.h

@@ -4226,7 +4226,7 @@ class gc_heap
 
 #ifdef DYNAMIC_HEAP_COUNT
     // Sample collection -
-    //
+    // 
     // For every GC, we collect the msl wait time + GC pause duration info and use both to calculate the
     // throughput cost percentage. We will also be using the wait time and the GC pause duration separately
     // for other purposes in the future.

src/coreclr/vm/amd64/JitHelpers_InlineGetThread.asm

@@ -41,7 +41,7 @@ LEAF_ENTRY JIT_TrialAllocSFastMP_InlineGetThread, _TEXT
         ; m_BaseSize is guaranteed to be a multiple of 8.
 
         INLINE_GETTHREAD r11
-        mov     r10, [r11 + OFFSET__Thread__m_alloc_context__alloc_sampling]
+        mov     r10, [r11 + OFFSET__Thread__m_alloc_context__combined_limit]
         mov     rax, [r11 + OFFSET__Thread__m_alloc_context__alloc_ptr]
 
         add     rdx, rax
@@ -65,7 +65,7 @@ NESTED_ENTRY JIT_BoxFastMP_InlineGetThread, _TEXT
         mov     r8d, [rcx + OFFSET__MethodTable__m_BaseSize]
 
         INLINE_GETTHREAD r11
-        mov     r10, [r11 + OFFSET__Thread__m_alloc_context__alloc_sampling]
+        mov     r10, [r11 + OFFSET__Thread__m_alloc_context__combined_limit]
         mov     rax, [r11 + OFFSET__Thread__m_alloc_context__alloc_ptr]
 
         add     r8, rax
@@ -136,7 +136,7 @@ LEAF_ENTRY AllocateStringFastMP_InlineGetThread, _TEXT
         and     edx, -8
 
         INLINE_GETTHREAD r11
-        mov     r10, [r11 + OFFSET__Thread__m_alloc_context__alloc_sampling]
+        mov     r10, [r11 + OFFSET__Thread__m_alloc_context__combined_limit]
         mov     rax, [r11 + OFFSET__Thread__m_alloc_context__alloc_ptr]
 
         add     rdx, rax
@@ -189,7 +189,7 @@ LEAF_ENTRY JIT_NewArr1VC_MP_InlineGetThread, _TEXT
 
 
         INLINE_GETTHREAD r11
-        mov     r10, [r11 + OFFSET__Thread__m_alloc_context__alloc_sampling]
+        mov     r10, [r11 + OFFSET__Thread__m_alloc_context__combined_limit]
         mov     rax, [r11 + OFFSET__Thread__m_alloc_context__alloc_ptr]
 
         add     r8, rax
@@ -238,7 +238,7 @@ LEAF_ENTRY JIT_NewArr1OBJ_MP_InlineGetThread, _TEXT
         ; to be a multiple of 8.
 
         INLINE_GETTHREAD r11
-        mov     r10, [r11 + OFFSET__Thread__m_alloc_context__alloc_sampling]
+        mov     r10, [r11 + OFFSET__Thread__m_alloc_context__combined_limit]
         mov     rax, [r11 + OFFSET__Thread__m_alloc_context__alloc_ptr]
 
         add     r8, rax

src/coreclr/vm/amd64/JitHelpers_Slow.asm

@@ -184,7 +184,7 @@ LEAF_ENTRY JIT_TrialAllocSFastSP, _TEXT
         jnz     JIT_NEW
 
         mov     rax, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__alloc_ptr]      ; alloc_ptr
-        mov     r10, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__alloc_sampling] ; alloc_sampling
+        mov     r10, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__combined_limit] ; combined_limit
 
         add     r8, rax
 
@@ -212,7 +212,7 @@ NESTED_ENTRY JIT_BoxFastUP, _TEXT
         jnz     JIT_Box
 
         mov     rax, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__alloc_ptr]      ; alloc_ptr
-        mov     r10, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__alloc_sampling] ; alloc_sampling
+        mov     r10, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__combined_limit] ; combined_limit
 
         add     r8, rax
 
@@ -291,7 +291,7 @@ LEAF_ENTRY AllocateStringFastUP, _TEXT
         jnz     FramedAllocateString
 
         mov     rax, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__alloc_ptr]      ; alloc_ptr
-        mov     r10, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__alloc_sampling] ; alloc_sampling
+        mov     r10, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__combined_limit] ; combined_limit
 
         add     r8, rax
 
@@ -347,7 +347,7 @@ LEAF_ENTRY JIT_NewArr1VC_UP, _TEXT
         jnz     JIT_NewArr1
 
         mov     rax, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__alloc_ptr]      ; alloc_ptr
-        mov     r10, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__alloc_sampling] ; alloc_sampling
+        mov     r10, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__combined_limit] ; combined_limit
 
         add     r8, rax
         jc      AllocFailed
@@ -400,7 +400,7 @@ LEAF_ENTRY JIT_NewArr1OBJ_UP, _TEXT
         jnz     JIT_NewArr1
 
         mov     rax, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__alloc_ptr]      ; alloc_ptr
-        mov     r10, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__alloc_sampling] ; alloc_sampling
+        mov     r10, [g_global_ee_alloc_context + OFFSETOF__ee_alloc_context__combined_limit] ; combined_limit
 
         add     r8, rax
 

src/coreclr/vm/amd64/asmconstants.h

@@ -122,14 +122,14 @@ ASMCONSTANTS_C_ASSERT(OFFSETOF__Thread__m_pFrame
 #define               OFFSET__Thread__m_alloc_context__alloc_ptr 0x60
 ASMCONSTANTS_C_ASSERT(OFFSET__Thread__m_alloc_context__alloc_ptr == offsetof(Thread, m_alloc_context) + offsetof(ee_alloc_context, gc_alloc_context) + offsetof(gc_alloc_context, alloc_ptr));
 
-#define               OFFSET__Thread__m_alloc_context__alloc_sampling 0x58
-ASMCONSTANTS_C_ASSERT(OFFSET__Thread__m_alloc_context__alloc_sampling == offsetof(Thread, m_alloc_context) + offsetof(ee_alloc_context, alloc_sampling));
+#define               OFFSET__Thread__m_alloc_context__combined_limit 0x58
+ASMCONSTANTS_C_ASSERT(OFFSET__Thread__m_alloc_context__combined_limit == offsetof(Thread, m_alloc_context) + offsetof(ee_alloc_context, combined_limit));
 
 #define               OFFSETOF__ee_alloc_context__alloc_ptr 0x8
 ASMCONSTANTS_C_ASSERT(OFFSETOF__ee_alloc_context__alloc_ptr == offsetof(ee_alloc_context, gc_alloc_context) + offsetof(gc_alloc_context, alloc_ptr));
 
-#define               OFFSETOF__ee_alloc_context__alloc_sampling 0x0
-ASMCONSTANTS_C_ASSERT(OFFSETOF__ee_alloc_context__alloc_sampling == offsetof(ee_alloc_context, alloc_sampling));
+#define               OFFSETOF__ee_alloc_context__combined_limit 0x0
+ASMCONSTANTS_C_ASSERT(OFFSETOF__ee_alloc_context__combined_limit == offsetof(ee_alloc_context, combined_limit));
 
 #define               OFFSETOF__ThreadExceptionState__m_pCurrentTracker 0x000
 ASMCONSTANTS_C_ASSERT(OFFSETOF__ThreadExceptionState__m_pCurrentTracker

src/coreclr/vm/frozenobjectheap.cpp

@@ -112,7 +112,7 @@ Object* FrozenObjectHeapManager::TryAllocateObject(PTR_MethodTable type, size_t
 
     } // end of GCX_PREEMP
 
-    PublishFrozenObject(obj, objectSize);
+    PublishFrozenObject(obj);
 
     return obj;
 #endif // !FEATURE_BASICFREEZE

src/coreclr/vm/gcenv.ee.cpp

@@ -446,26 +446,28 @@ gc_alloc_context * GCToEEInterface::GetAllocContext()
 
 void InvokeGCAllocCallback(ee_alloc_context* pEEAllocContext, enum_alloc_context_func* fn, void* param)
 {
-    // NOTE: Its possible that alloc_ptr = alloc_limit = alloc_sampling = NULL at this point
+    // NOTE: Its possible that alloc_ptr = alloc_limit = combined_limit = NULL at this point
     gc_alloc_context* pAllocContext = &pEEAllocContext->gc_alloc_context;
 
     // The allocation context might be modified by the callback, so we need to save
     // the remaining sampling budget and restore it after the callback if needed.
-    size_t currentSamplingBudget = (size_t)(pEEAllocContext->alloc_sampling - pAllocContext->alloc_ptr);
+    size_t currentSamplingBudget = (size_t)(pEEAllocContext->combined_limit - pAllocContext->alloc_ptr);
     size_t currentSize = (size_t)(pAllocContext->alloc_limit - pAllocContext->alloc_ptr);
 
     fn(pAllocContext, param);
 
     // If the GC changed the size of the allocation context, we need to recompute the sampling limit
     // This includes the case where the AC was initially zero-sized/uninitialized.
+    // Functionally we'd get valid results if we called UpdateCombinedLimit() unconditionally but its
+    // empirically a little more performant to only call it when the AC size has changed.
     if (currentSize != (size_t)(pAllocContext->alloc_limit - pAllocContext->alloc_ptr))
     {
-        pEEAllocContext->ComputeSamplingLimit(GetThread()->GetRandom());
+        pEEAllocContext->UpdateCombinedLimit(GetThread()->GetRandom());
     }
     else
     {
         // Restore the remaining sampling budget as the size is the same.
-        pEEAllocContext->alloc_sampling = pAllocContext->alloc_ptr + currentSamplingBudget;
+        pEEAllocContext->combined_limit = pAllocContext->alloc_ptr + currentSamplingBudget;
     }
 }
 

src/coreclr/vm/gcheaputilities.h

@@ -19,59 +19,74 @@ const DWORD SamplingDistributionMean = (100 * 1024);
 // This struct adds some state that is only visible to the EE onto the standard gc_alloc_context
 typedef struct _ee_alloc_context
 {
-    // This is the address where the next allocation sampling should occur:
-    //  - if sampling is off, this is the same as alloc_limit
-    //  - if sampling is on, this is the next sampled byte in the gc_alloc_context
-    //  - if no such byte exists (i.e. the sampling threshold is outside of the allocation context),
-    //     this is the same as alloc_limit
-    uint8_t* alloc_sampling;
+    // Any allocation that would overlap combined_limit needs to be handled by the allocation slow path.
+    // combined_limit is the minimum of:
+    //  - gc_alloc_context.alloc_limit (the end of the current AC)
+    //  - the sampling_limit
+    //
+    // In the simple case that randomized sampling is disabled, combined_limit is always equal to alloc_limit.
+    //
+    // There are two different useful interpretations for the sampling_limit. One is to treat the sampling_limit
+    // as an address and when we allocate an object that overlaps that address we should emit a sampling event.
+    // The other is that we can treat (sampling_limit - alloc_ptr) as a budget of how many bytes we can allocate
+    // before emitting a sampling event. If we always allocated objects contiguously in the AC and incremented
+    // alloc_ptr by the size of the object, these two interpretations would be equivalent. However, when objects
+    // don't fit in the AC we allocate them in some other address range. The budget interpretation is more
+    // flexible to handle those cases.
+    //
+    // The sampling limit isn't stored in any separate field explicitly, instead it is implied:
+    // - if combined_limit == alloc_limit there is no sampled byte in the AC. In the budget interpretation
+    //   we can allocate (alloc_limit - alloc_ptr) unsampled bytes. We'll need a new random number after
+    //   that to determine whether future allocated bytes should be sampled.
+    //   This occurs either because the sampling feature is disabled, or because the randomized selection
+    //   of sampled bytes didn't select a byte in this AC.
+    // - if combined_limit < alloc_limit there is a sample limit in the AC. sample_limit = combined_limit.
+    uint8_t* combined_limit;
     gc_alloc_context gc_alloc_context;
 
  public:
     void init()
     {
         LIMITED_METHOD_CONTRACT;
-
-        // we can't compute a sampling limit
-        // because we don't know the size of the allocation context yet
-        alloc_sampling = nullptr;
+        combined_limit = nullptr;
         gc_alloc_context.init();
     }
 
-    inline void ComputeSamplingLimit(CLRRandom* pRandomizer)
+    static inline bool IsRandomizedSamplingEnabled()
     {
-        // the caller of this function does not have to check if sampling is on/off
-        // If sampling is off, this is just setting alloc_sampling = alloc_limit
-        // If sampling is on then we'd do some pseudo-random number generation to decide what is
-        // the next sampled byte in the gc_alloc_context, if any.
-        if (ETW_TRACING_CATEGORY_ENABLED(MICROSOFT_WINDOWS_DOTNETRUNTIME_PROVIDER_DOTNET_Context,
-                                         TRACE_LEVEL_INFORMATION,
-                                         CLR_ALLOCATIONSAMPLING_KEYWORD))
-        {
-            // compute the next sampling limit based on a geometric distribution
-            size_t threshold = ComputeGeometricRandom(pRandomizer);
+        return ETW_TRACING_CATEGORY_ENABLED(MICROSOFT_WINDOWS_DOTNETRUNTIME_PROVIDER_DOTNET_Context,
+                                        TRACE_LEVEL_INFORMATION,
+                                        CLR_ALLOCATIONSAMPLING_KEYWORD);
+    }
+
+    // Regenerate the randomized sampling limit and update the combined_limit field.
+    inline void UpdateCombinedLimit(CLRRandom* pRandom)
+    {
+        UpdateCombinedLimit(IsRandomizedSamplingEnabled(), pRandom);
+    }
 
-            // if the threshold is larger than the allocation context, no sampling will occur
-            alloc_sampling = Min(gc_alloc_context.alloc_ptr + threshold, gc_alloc_context.alloc_limit);
+    // Regenerate the randomized sampling limit and update the combined_limit field.
+    inline void UpdateCombinedLimit(bool samplingEnabled, CLRRandom* pRandom)
+    {
+        if (!samplingEnabled)
+        {
+            combined_limit = gc_alloc_context.alloc_limit;
         }
         else
         {
-            alloc_sampling = gc_alloc_context.alloc_limit;
-        }
-    }
+            // compute the next sampling limit based on a geometric distribution
+            uint8_t* sampling_limit = gc_alloc_context.alloc_ptr + ComputeGeometricRandom(pRandom);
 
-    // it is expected that the caller of these functions has already checked if sampling is on/off
-    static inline bool IsSampled(CLRRandom* pRandomizer, size_t range)
-    {
-        size_t threshold = ComputeGeometricRandom(pRandomizer);
-        return (threshold < range);
+            // if the sampling limit is larger than the allocation context, no sampling will occur in this AC
+            combined_limit = Min(sampling_limit, gc_alloc_context.alloc_limit);
+        }
     }
 
-    static inline size_t ComputeGeometricRandom(CLRRandom* pRandomizer)
+    static inline int ComputeGeometricRandom(CLRRandom* pRandomizer)
     {
         // compute a random sample from the Geometric distribution
         double probability = pRandomizer->NextDouble();
-        size_t threshold = (size_t)(-log(1 - probability) * SamplingDistributionMean);
+        int threshold = (int)(-log(1 - probability) * SamplingDistributionMean);
         return threshold;
     }
 } ee_alloc_context;