8000 Optimize assembly/namespace pair creation and updating in XamlSchema by h3xds1nz · Pull Request #9926 · dotnet/wpf · GitHub
[go: up one dir, main page]
Skip to content

Optimize assembly/namespace pair creation and updating in XamlSchema #9926

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 6 commits into from
May 16, 2025
Merged

Optimize assembly/namespace pair creation and updating in XamlSchema #9926

merged 6 commits into from
May 16, 2025

Conversation

h3xds1nz
Copy link
Member
@h3xds1nz h3xds1nz commented Oct 10, 2024 •
edited
Loading

Description

Adjusts inefficient List<AssemblyNamespacePair> copies in AddAssemblyNamespacePair that result in up to double gen0 allocations needed for the job, offering perf benefit and decreased allocations.

  • I have changed it to be a readonly struct, there's like zero benefit but I think it serves better purpose.
  • The WeakReference is now generic to prevent unnecessary casting.
  • Since _assemblyNamespaces is under a write-lock and copies are created each time to allow reading concurrently by multiple readers, we can just swap to an array for overall efficiency since only 1 item is added each time.

Initial creation

Method Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original 108.37 ns 2.179 ns 2.594 ns 0.0086 317 B 144 B
PR_EDIT 89.78 ns 1.833 ns 2.687 ns 0.0038 223 B 64 B

Initial creation plus 9x AddAssemblyNamespacePair

Method Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original 1,419.8 ns 28.21 ns 37.66 ns 0.1678 2,668 B 2824 B
PR_EDIT 1,100.8 ns 18.28 ns 16.20 ns 0.0935 2,329 B 1584 B

Customer Impact

Improved performance, decreased allocations.

Regression

No.

Testing

Local build, basic method testing.

Risk

Low, there's only a swap from class to readonly struct being stored in a private array (that was originally a list).

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners October 10, 2024 11:14
@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 Oct 10, 2024
@h3xds1nz h3xds1nz force-pushed the namespace-pair-as-struct branch 2 times, most recently from a1c2ab7 to 2e0f1d6 Compare January 28, 2025 15:06
@h3xds1nz
Copy link
Member Author

@harshit7962 @himgoyalmicro I've fixed the dummy NRE mistake in the last commit, so this could finally go through tests.

@dipeshmsft
Copy link
Member

@h3xds1nz, this PR looks good to me. Can you resolve the merge conflicts here ?

PS: From what I gather, copying a List is heavier as compared to array and that's why the shift ? If yes, then maybe we can add this as a comment where AssemblyNamespacePair[] array is defined.

@dipeshmsft dipeshmsft self-assigned this May 16, 2025
@h3xds1nz
Copy link
Member Author
h3xds1nz commented May 16, 2025 •
edited
Loading

@dipeshmsft List<T> is merely a wrapper over T[] which in normal scenarios (besides ref returns etc.) doesn't really add much overhead over array and will often be more beneficial overall if you need to add items and resize them as you already pre-allocate some scratch space but access it as a limited-number array.

This is a special case, or rather, lazy misuse of List<T> capabilities as you:

  1. only build the list at the beginning
  2. only add 1 item per iteration
  3. you're attempting atomic updates, so you're forced to create a new instance each time

This means you're here initializing the List<T> from another List<T>, which creates a precisely-sized array onto you which then add a new item which incurs a new, practically redundant, copy of the just freshly created List<T>.

Therefore exploiting the fact that we only add 1 item, we can just allocate an array with n + 1 elements, copy then n and add the 1, without having to copy twice.

Plus we get better control of the sizing (empty list on first addition allocates array or 4 vs our array of 1) and then we don't grow by * 2 number of items but by 1.

@dipeshmsft
Copy link
Member

This means you're here initializing the List from another List, which creates a precisely-sized array onto you which then add a new item which incurs a new, practically redundant, copy of the just freshly created List.

Okay, so now using array, we pre-allocate the extra space for the 1 extra element, preventing the extra copy. Thanks a lot for the explaination.

@h3xds1nz h3xds1nz force-pushed the namespace-pair-as-struct branch from 2e0f1d6 to 339993a Compare May 16, 2025 09:11
@h3xds1nz
Copy link
Member Author

@dipeshmsft Anytime :) (though usually it is better to answer questions regarding PRs in the next few weeks than 8 months later, one might miss context after that time if its something complicated, which isn't the case here but couldn't help myself to nitpick about the timeline 😀)

Conflicts resolved.

@codecov Codecov
Copy link
codecov bot commented May 16, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 13.47617%. Comparing base (c572f8e) to head (339993a).
Report is 8 commits behind head on main.

Additional details and impacted files 
@@                 Coverage Diff                 @@
##                main       #9926         +/-   ##
===================================================
- Coverage   13.62756%   13.47617%   -0.15140%     
===================================================
  Files           3316        3317          +1     
  Lines         664822      664751         -71     
  Branches       74651       74643          -8     
===================================================
- Hits           90599       89583       -1016     
- Misses        571523      572569       +1046     
+ Partials        2700        2599        -101
Flag Coverage Δ
Debug 13.47617% <92.30769%> (-0.15140%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dipeshmsft dipeshmsft merged commit 41d71f5 into dotnet:main May 16, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dipeshmsft dipeshmsft dipeshmsft approved these changes

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

Assignees

@h3xds1nz h3xds1nz

@dipeshmsft dipeshmsft

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 Status:Proposed
Projects
None yet
Milestone
No milestone
Development

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