8000 Replace occurrences of params T[] with params ReadOnlySpan<T> where possible by h3xds1nz · Pull Request #9481 · dotnet/wpf · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jan 8, 2025
Merged

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

merged 11 commits into from
Jan 8, 2025

Conversation

h3xds1nz
Copy link
Member
@h3xds1nz h3xds1nz commented Jul 28, 2024 •
edited
Loading

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 mentioned this pull request Sep 23, 2024
Merged
@h3xds1nz h3xds1nz force-pushed the replace-params-with-span branch from 714abef to 1217ef8 Compare September 23, 2024 18:40
@h3xds1nz
Copy link
Member 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
Member Author
h3xds1nz commented Oct 7, 2024

Resolved merge conflicts 09

@h3xds1nz h3xds1nz mentioned this pull request Oct 31, 2024
Open
@h3xds1nz h3xds1nz mentioned this pull request Jan 3, 2025
Merged
@harshit7962 harshit7962 self-assigned this Jan 8, 2025
harshit7962
harshit7962 previously approved these changes Jan 8, 2025
View reviewed changes
Copy link
Member
@harshit7962 harshit7962 left a comment

Choose a reason for hiding this comment

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

The changes LGTM. @h3xds1nz, the changes can be taken in after resolving the conflicts.

@h3xds1nz
another few safe swaps from params object[] to params ReadOnlySpan<ob…
ac9e62a 
…ject>
@h3xds1nz
convert VirtualizingStackPanel from params object[] to params ReadOnl…
8d8cde1 
…ySpan<object>
@h3xds1nz
convert TextStore from params object[] to params ReadOnlySpan<object>
41420ee
@h3xds1nz
convert CommandHelpers from params InputGesture[] to params ReadOnlyS…
1350b7d 
…pan<InputGesture>
@h3xds1nz
convert VisualStates transitions from params string[] to params ReadO…
e25d398 
…nlySpan<string>
@h3xds1nz
convert VisualStateGroup from params Storyboard[] to params ReadOnlyS…
3396314 
…pan<Storyboard>
@h3xds1nz h3xds1nz force-pushed the replace-params-with-span branch from 10775f7 to c451e92 Compare January 8, 2025 11:59
harshit7962
harshit7962 approved these changes Jan 8, 2025
View reviewed changes
Copy link
Member
@harshit7962 harshit7962 left a comment

Choose a reason for hiding this comment

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

Re-approving post conflict resolution.

@harshit7962 harshit7962 merged commit 648de00 into dotnet:main Jan 8, 2025
8 checks passed
@harshit7962
Copy link
Member

Thanks @h3xds1nz for the contribution.

@h3xds1nz
Copy link
Member Author
h3xds1nz commented Jan 8, 2025

@harshit7962 Thanks, this one is big.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dipeshmsft dipeshmsft dipeshmsft left review comments

@harshit7962 harshit7962 harshit7962 approved these changes

@singhashish-wpf singhashish-wpf Awaiting requested review from singhashish-wpf

Assignees

@harshit7962 harshit7962

@h3xds1nz h3xds1nz

Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@h3xds1nz @harshit7962 @dipeshmsft
0