Skip to content

Inline GC handle write barrier - #132245

Open
jkotas with Copilot wants to merge 18 commits into
mainfrom
copilot/improve-gchandle-target-performance
Open

Inline GC handle write barrier#132245
jkotas with Copilot wants to merge 18 commits into
mainfrom
copilot/improve-gchandle-target-performance

Conversation

Copilot AI commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

Validation

  • Built the CoreCLR GC native target successfully.
  • Built libraries successfully.
  • Full CoreCLR Release build reached the changed GC objects successfully but failed later on pre-existing JIT identifier errors in instrsxarch.h.
  • The focused GCHandle.Target test could not be built because test dependency assets have not been generated.

Performance

  • EgorBot measurement requested for Linux x64 and macOS ARM64.

Note

This pull request description was generated by AI/Copilot.

Copilot AI and others added 2 commits August 12, 2026 23:10
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 12, 2026 23:22
@azure-pipelines

Copy link
Copy Markdown
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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @anicka-net, @dotnet/gc
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 HndWriteBarrierWorker into src/coreclr/gc/handletable.inl and remove the out-of-line implementation from handletable.cpp.
  • Add an early-return fast-path in the worker to skip HandleFetchType(handle) when GetConvertedGeneration(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).

Comment thread src/coreclr/gc/handletable.h Outdated
Comment thread src/coreclr/gc/handletable.h
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 13, 2026 05:51
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Copilot AI review requested due to automatic review settings August 13, 2026 06:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • HandleFetchType in handletable.inl reads the block type via raw uint8_t* indexing (segment[HANDLE_SEGMENT_BLOCK_TYPE_OFFSET + uBlock]). The existing offsetof static_assert guards the offset, but the implementation also assumes rgBlockType elements 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.inl uses C-style casts when deriving segment pointers (e.g., (uint8_t*)((uintptr_t)handle & HANDLE_SEGMENT_ALIGN_MASK)), while nearby code already uses reinterpret_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);

Comment thread src/coreclr/gc/gcinternal.h
Comment thread src/coreclr/gc/handletablepriv.h Outdated
Comment thread src/coreclr/gc/handletablepriv.h Outdated
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 17, 2026 21:04
Copilot AI requested a review from jkotas August 17, 2026 21:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

…leHandleFromCache

Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 17, 2026 21:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/coreclr/gc/handletable.inl:256

  • The #endif comment doesn’t match the corresponding #ifndef DACCESS_COMPILE (repo native guidelines request matching #endif comments). This makes preprocessor blocks harder to audit in low-level code.
#endif // DACCESS_COMPILE

Comment thread src/coreclr/gc/handletablepriv.h
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 17, 2026 22:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

@jkotas

jkotas commented Aug 17, 2026

Copy link
Copy Markdown
Member

@EgorBot -linux_x64

using System.Runtime.InteropServices;
using BenchmarkDotNet.Attributes;

public class Bench
{
    [Benchmark]
    public void GCHandle()
    {
        using (var hnd = new PinnedGCHandle<string>("Hello"))
        {
        }
    }
}

@EgorBo

EgorBo commented Aug 17, 2026

Copy link
Copy Markdown
Member

@EgorBot -linux_x64 -profiler

using System.Runtime.InteropServices;
using BenchmarkDotNet.Attributes;

public class Bench
{
    [Benchmark]
    public void GCHandle()
    {
        using (var hnd = new PinnedGCHandle<string>("Hello"))
        {
        }
    }
}

Comment thread src/coreclr/gc/handletable.inl Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Make sure HndAssignHandle is inlined

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 9ef9134 by changing HndAssignHandle to FORCEINLINE. Verified ./build.sh clr.runtime and ./build.sh clr.runtime -c Release; the Release handle-table/GC objects no longer contain an out-of-line HndAssignHandle symbol.

Note

This reply was generated by AI/Copilot.

Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants