Skip to content
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 ee_alloc_context #104849

Merged
merged 7 commits into from
Oct 15, 2024
Merged

Add ee_alloc_context #104849

merged 7 commits into from
Oct 15, 2024

Commits on Oct 15, 2024

  1. Add ee_alloc_context

    This change is some preparatory refactoring for the randomized allocation sampling feature. We need to add more state onto allocation context but we don't want to do a breaking change of the GC interface. The new state only needs to be visible to the EE but we want it physically near the existing alloc context state for good cache locality. To accomplish this we created a new ee_alloc_context struct which contains an instance of gc_alloc_context within it.
    
    The new ee_alloc_context.combined_limit field should be used by fast allocation helpers to determine when to go down the slow path. Most of the time combined_limit has the same value as alloc_limit, but periodically we need to emit an allocation sampling event on an object that is somewhere in the middle of an AC. Using combined_limit rather than alloc_limit as the slow path trigger allows us to keep all the sampling event logic in the slow path.
    noahfalk committed Oct 15, 2024
    Configuration menu
    Copy the full SHA
    e4bf1a4 View commit details
    Browse the repository at this point in the history
  2. PR feedback

    combined_limit is now synchronized in GcEnumAllocContexts instead of RestartEE.
    This requires the GC being constrained in how it updates the alloc_ptr and alloc_limit. No GC behavior changed,
    in practice, but the constraints are now part of the EE<->GC contract so that we can rely on them in the EE code.
    noahfalk committed Oct 15, 2024
    Configuration menu
    Copy the full SHA
    8471d60 View commit details
    Browse the repository at this point in the history
  3. Apply suggestions from code review

    Co-authored-by: Jan Kotas <[email protected]>
    noahfalk and jkotas committed Oct 15, 2024
    Configuration menu
    Copy the full SHA
    5988bce View commit details
    Browse the repository at this point in the history
  4. Apply suggestions from code review

    Co-authored-by: Jan Kotas <[email protected]>
    noahfalk and jkotas committed Oct 15, 2024
    Configuration menu
    Copy the full SHA
    897212c View commit details
    Browse the repository at this point in the history
  5. Fix dac-ization bug

    The code of GetAllocContext() was constructing a PTR_gc_alloc_context which does a host->target pointer conversion. Those conversions work by doing a lookup in a dictionary of blocks of memory that we have previously marshalled and the pointer being converted is expected to be the start of the memory block. In this case we had never previously marshalled the gc_allocation_context on its own. We had only marshalled the m_pRuntimeThreadLocals block which includes the gc_allocation_context inside of it at a non-zero offset. This caused the host->target pointer conversion to fail which in turn meant commands like !threads in SOS would fail.
    
    The fix is pretty trivial. We don't need to do a host->target conversion here at all because the calling code in the DAC is going to immediately convert right back to a host pointer. We can avoid the conversion in both directions by eliminating the cast and returning the host pointer directly.
    noahfalk committed Oct 15, 2024
    Configuration menu
    Copy the full SHA
    074dc0e View commit details
    Browse the repository at this point in the history
  6. Fix test failure

    Somehow a test reached Thread::CooperativeCleanup() with m_pRuntimeThreadLocals==NULL. Looking at the code I expected that would mean GetThread()==NULL or ThreadState contains TS_Dead, but neither of those conditions were true so it is unclear how it executed to that state. The callstack was:
    >	coreclr.dll!Thread::CooperativeCleanup() Line 2762	C++
     	coreclr.dll!Thread::DetachThread(int fDLLThreadDetach) Line 936	C++
     	coreclr.dll!TlsDestructionMonitor::~TlsDestructionMonitor() Line 1745	C++
     	coreclr.dll!__dyn_tls_dtor(void * __formal, const unsigned long dwReason, void * __formal) Line 122	C++
     	ntdll.dll!_LdrxCallInitRoutine@16()	Unknown
     	ntdll.dll!LdrpCallInitRoutine()	Unknown
     	ntdll.dll!LdrpCallTlsInitializers()	Unknown
     	ntdll.dll!LdrShutdownThread()	Unknown
     	ntdll.dll!RtlExitUserThread()	Unknown
    
    Regardless, the previous code wouldn't have hit that issue because it obtained the pointer through t_runtime_thread_locals rather than m_pRuntimeThreadLocals. I restored using t_runtime_thread_locals in the Cleanup routine. Out of caution I also searched for any other places that were previously accessing the alloc_context through the thread local address and ensured they don't switch to use m_pRuntimeThreadLocals either.
    noahfalk committed Oct 15, 2024
    Configuration menu
    Copy the full SHA
    e1771be View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    c275f4b View commit details
    Browse the repository at this point in the history