Conversation
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @anicka-net, @dotnet/gc |
There was a problem hiding this comment.
Pull request overview
This PR refactors the CoreCLR GC handle write-barrier by moving the HndWriteBarrierWorker implementation into handletable.inl (so it can be inlined into its callers) and introduces a fast-path that avoids the handle-type lookup when the assigned target is in generation 0. This targets reduced overhead in hot handle assignment paths while keeping the GC/EE interface shape unchanged.
Changes:
- Inline
HndWriteBarrierWorkerintosrc/coreclr/gc/handletable.inland remove the out-of-line implementation fromhandletable.cpp. - Add an early-return fast-path in the worker to skip
HandleFetchType(handle)whenGetConvertedGeneration(value)is 0. - Move/adjust declarations (
GetConvertedGeneration,HandleFetchType) to be available to the inlined implementation and update includes accordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/gc/handletablepriv.h | Removes now-unneeded forward declarations previously used by the out-of-line worker. |
| src/coreclr/gc/handletable.inl | Adds the inlined HndWriteBarrierWorker implementation and updates HndAssignHandle to call it directly. |
| src/coreclr/gc/handletable.h | Adds handletableconstants.h include and exposes needed non-DAC prototypes for the inlined worker. |
| src/coreclr/gc/handletable.cpp | Removes the out-of-line HndWriteBarrierWorker implementation (now provided inline). |
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/gc/handletable.inl:68
- The comment describing async pinned handle behavior has a few grammar/wording issues (e.g., missing space after
//, “need”/“consider”). Updating it will make the intent clearer without changing behavior.
//OverlappedData need special treatment: because all user data pointed by it needs to be reported by this handle,
//its age is consider to be min age of the user data, to be simple, we just make it 0
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/coreclr/gc/handletablepriv.h:171
HandleFetchTypeinhandletable.inlreads the block type via rawuint8_t*indexing (segment[HANDLE_SEGMENT_BLOCK_TYPE_OFFSET + uBlock]). The existingoffsetofstatic_assert guards the offset, but the implementation also assumesrgBlockTypeelements are 1 byte. Adding a size check here makes that assumption explicit and prevents silent breakage if the field type ever changes.
static_assert(offsetof(_TableSegmentHeader, rgBlockType) == HANDLE_SEGMENT_BLOCK_TYPE_OFFSET);
src/coreclr/gc/handletable.inl:43
- New code in
handletable.inluses C-style casts when deriving segment pointers (e.g.,(uint8_t*)((uintptr_t)handle & HANDLE_SEGMENT_ALIGN_MASK)), while nearby code already usesreinterpret_cast. This is easy to fix and aligns with the repo's native guidance to prefer C++ casts.
uint8_t* barrier = (uint8_t*)((uintptr_t)handle & HANDLE_SEGMENT_ALIGN_MASK);
_ASSERTE(barrier);
uintptr_t offset = (uintptr_t)handle & HANDLE_SEGMENT_CONTENT_MASK;
_ASSERTE(offset >= HANDLE_HEADER_SIZE);
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/gc/handletable.inl:70
- Fix grammar/spacing in this comment (missing space after
//, subject/verb agreement, and "consider" → "considered") to keep comments clear and consistent.
//OverlappedData need special treatment: because all user data pointed by it needs to be reported by this handle,
//its age is consider to be min age of the user data, to be simple, we just make it 0
|
@EgorBot -windows_x64 -linux_x64 -linux_arm64 |
|
@copilot Delete TableFreeHandlesToCache method - it is dead code |
|
@copilot Make sure that TableFreeSingleHandleToCache is inlined in the one place it is called |
|
@copilot GetGlobalHandleStore never fails. Delete unnecessary error handling in appdomain.cpp |
|
@copilot Make sure that GetGlobalHandleStore iis cached in a static in NativeAOT - similar to how it is done in CoreCLR |
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/coreclr/gc/handletable.inl:71
- The comment for async pinned handles has grammar issues and is a bit hard to read (e.g., "need"/"is consider"), which makes the rationale unclear in a subtle write-barrier path. Consider rewording for clarity while keeping the intent the same.
//OverlappedData need special treatment: because all user data pointed by it needs to be reported by this handle,
//its age is consider to be min age of the user data, to be simple, we just make it 0
if (uType == HNDTYPE_ASYNCPINNED)
…e GetGlobalHandleStore in NativeAOT Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Added
Removed the null check and
Addressed in 55c837b — moved |
| // Retrieves the global GC handle store. | ||
| static IGCHandleStore* GetGlobalHandleStore() | ||
| { | ||
| LIMITED_METHOD_CONTRACT; | ||
|
|
||
| assert(g_pGlobalHandleStore != nullptr); | ||
| return g_pGlobalHandleStore; | ||
| } |
Summary
Validation
instrsxarch.h.GCHandle.Targettest could not be built because test dependency assets have not been generated.Performance
Note
This pull request description was generated by AI/Copilot.