Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Replace occurrences of params T[] with params ReadOnlySpan<T> where possible #9481

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

h3xds1nz
Copy link
Contributor

@h3xds1nz h3xds1nz commented Jul 28, 2024

Description

Continues the journey that started in #9468 and converts some internal call-chains from params T[] to params ReadOnlySpan<T> where I felt it is without any side-effects, largely decreasing allocations in other scenarios besides just tracing and improving both performance and allocations since we do not need to create a new heap array of T[] everytime.

Simple benchmark with 0, 1, and 3 params

Method Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original 7.547 ns 0.1129 ns 0.1056 ns 0.0029 143 B 48 B
PR__EDIT 1.532 ns 0.0096 ns 0.0085 ns - 120 B -
Benchmark code
private static object target1;
private static object target2;
private static object target3;

[GlobalSetup]
public void Setup()
{
    target1 = new object();
    target2 = new object();
    target3 = new object();
}

[Benchmark]
public int Original()
{
    int test1_1 = Test1(1);
    int test1_2 = Test1(1, null);
    int test1_3 = Test1(1, target1, target2, target3);

    return  test1_1 + test1_2 + test1_3;
}


[Benchmark]
public int PR__EDIT()
{
    int test2_1 = Test2(1);
    int test2_2 = Test2(1, null);
    int test2_3 = Test2(1, target1, target2, target3);

    return test2_1 + test2_2 + test2_3;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private int Test1(int stuff, params object[] args)
{
    if (args is null || args.Length == 0)
        return stuff;

    return args[0].GetHashCode();
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private int Test2(int stuff, params ReadOnlySpan<object> args)
{
    if (args.IsEmpty)
        return stuff;

    return args[0].GetHashCode();
}

Customer Impact

Improved performance, decreased allocations.

Regression

No.

Testing

Local build, test app run.

Risk

Low.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners July 28, 2024 16:45
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Jul 28, 2024
@h3xds1nz h3xds1nz force-pushed the replace-params-with-span branch from 714abef to 1217ef8 Compare September 23, 2024 18:40
@h3xds1nz
Copy link
Contributor Author

I have fixed the merge conflict and rebased.

@h3xds1nz h3xds1nz force-pushed the replace-params-with-span branch from 1217ef8 to 10775f7 Compare October 7, 2024 16:30
@h3xds1nz
Copy link
Contributor Author

h3xds1nz commented Oct 7, 2024

Resolved merge conflicts 09

/// <remarks>THIS IS A SHARED FILE. PresentationFramework.Design.dll must be rebuilt if changed.</remarks>
/// </summary>
/// <summary> Names and helpers for visual states in the controls. </summary>
/// <remarks> THIS IS A SHARED FILE: PresentationFramework.Design.dll must be rebuilt if changed. </remarks>
Copy link
Member

@dipeshmsft dipeshmsft Oct 8, 2024

Choose a reason for hiding this comment

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

PresentationFramework.Design.dll looks like something, I will have to take a look at.

Copy link
Contributor Author

@h3xds1nz h3xds1nz Oct 8, 2024

Choose a reason for hiding this comment

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

@dipeshmsft There used to be PresentationFramework.VisualStudio.Design.dll with old VS (e.g. my 2010 installation has it) as a private assembly (it was providing the design-time support afaik?). But the new ones don't have it as the whole designer has changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way, if you find the remarks irrelevant (which I'm very much confident it is these days), then I'd remove it in this PR before merging it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dipeshmsft Any news?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Community Contribution A label for all community Contributions Included in test pass PR metadata: Label to tag PRs, to facilitate with triage Status:Completed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants