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

ARM64: avoid memory barrier in InlinedMemmoveGCRefsHelper if dest is not on heap #102084

Merged
merged 9 commits into from
May 12, 2024

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented May 10, 2024

Should speed up on-stack struct copies

using System.Runtime.CompilerServices;
using System.Text.Json;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(Bench).Assembly).Run(args);

public class Bench
{
    [Benchmark]
    public void DestNotInHeap()
    {
        Utf8JsonReader dst = default;
        Utf8JsonReader src = default;
        DoCopy(ref dst, src);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public void DoCopy(ref Utf8JsonReader dst, Utf8JsonReader src) => dst = src;
}

Linux-arm64 (Ampere):

| Method        | Mean     | Error    | StdDev   |
|-------------- |---------:|---------:|---------:|
| DestNotInHeap | 15.39 ns | 0.014 ns | 0.013 ns | (Main)
| DestNotInHeap | 11.46 ns | 0.010 ns | 0.008 ns | (PR)

Fixes #102044

Copy link
Contributor

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.

@EgorBo
Copy link
Member Author

EgorBo commented May 10, 2024

PS: Utf8JsonReader (and, afair, some other STJ structs) is quite big (192 bytes) and is byref-like (ref struct), so, presumably, it should benefit a lot from #9512

@EgorBo EgorBo marked this pull request as ready for review May 10, 2024 22:03
@EgorBo
Copy link
Member Author

EgorBo commented May 10, 2024

@jkotas I guess this is ready for review then?

@@ -182,6 +182,9 @@ internal static unsafe void Memmove<T>(ref T destination, ref T source, nuint el
#endif
internal static void BulkMoveWithWriteBarrier(ref byte destination, ref byte source, nuint byteCount)
{
if (byteCount == 0 || Unsafe.AreSame(ref destination, ref source))
Copy link
Member

Choose a reason for hiding this comment

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

This method seems to be structured to allow inlining of BulkMoveWithWriteBarrier at every callsite.

I think it would be better to keep these checks inside the non-inlined portion of the helpers. The empty buffer or same buffer are rare cases. We do not need to optimize for them by inlining these checks at every callsite.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

Ruihan-Yin pushed a commit to Ruihan-Yin/runtime that referenced this pull request May 30, 2024
…not on heap (dotnet#102084)

* don't put full barrier if dest is not on the heap

* Address feedback

---------

Co-authored-by: Jan Kotas <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Jun 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Perf] Linux/arm64: 4 Regressions on 5/3/2024 3:54:35 PM
2 participants