8000 Remove double lookups on SizeLimitedCache<K, V>, improve performance by h3xds1nz · Pull Request #9949 · dotnet/wpf · GitHub
[go: up one dir, main page]
Skip to content

Remove double lookups on SizeLimitedCache<K, V>, improve performance #9949

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 8 commits into from
Mar 6, 2025
Merged

Remove double lookups on SizeLimitedCache<K, V>, improve performance #9949

merged 8 commits into from
Mar 6, 2025

Conversation

h3xds1nz
Copy link
Member
@h3xds1nz h3xds1nz commented Oct 13, 2024 •
edited by dotnet-policy-service bot
Loading

Fixes #4828

Description

While checking out SizeLimitedCache<K, V> yesterday, I've noticed the double lookups on the dictionary and today I've found there's already a corresponding issue for it, hence we should fix it for optimal performance as this is used with glyphs/fonts.

  • Use throw helpers over creating new instance of ArgumentNullException directly.
  • Remove had even three lookups, so now it just has one.
  • Fields that could be readonly have been adjusted.
  • Sealed the class, proper naming conventions.
  • Node uses auto properties now.

10 capacity, 15 unique additions first, 20 lookups

Method Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original 988.4 ns 9.39 ns 7.84 ns 0.1183 2,045 B 1992 B
PR_EDIT 809.3 ns 9.18 ns 8.14 ns 0.1183 1,820 B 1992 B

Customer Impact

Improved perfomance.

Regression

No.

Testing

Local build, tests with the class.

Risk

Low, just basically compiler warnings.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners October 13, 2024 11:33
@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 13, 2024
@h3xds1nz h3xds1nz mentioned this pull request Jan 3, 2025
Merged
@h3xds1nz h3xds1nz force-pushed the size-limited-cache-lookup branch from bb29d57 to b2ff6b5 Compare February 27, 2025 19:50
@h3xds1nz
Copy link
Member Author

Rebased onto main.

@ThomasGoulet73
Copy link
Contributor

@h3xds1nz: Just a ping to let you know that the test failures in this PR were fixed in main, pulling main should fix the failures.

@h3xds1nz h3xds1nz force-pushed the size-limited-cache-lookup branch from b2ff6b5 to ebcfa79 Compare February 28, 2025 09:04
@codecov Codecov
Copy link
codecov bot commented Feb 28, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.

Project coverage is 10.95662%. Comparing base (d94671d) to head (ebcfa79).
Report is 19 commits behind head on main.

Additional details and impacted files 
@@                 Coverage Diff                 @@
##                main       #9949         +/-   ##
===================================================
- Coverage   10.99508%   10.95662%   -0.03847%     
===================================================
  Files           3215        3215                 
  Lines         648472      648448         -24     
  Branches       71534       71530          -4     
===================================================
- Hits           71300       71048        -252     
- Misses        576170      576410        +240     
+ Partials        1002         990         -12
Flag Coverage Δ
Debug 10.95662% <0.00000%> (-0.03847%) ⬇️

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.

@himgoyalmicro himgoyalmicro self-assigned this Mar 6, 2025
himgoyalmicro
himgoyalmicro approved these changes Mar 6, 2025
View reviewed changes
Copy link
Contributor
@himgoyalmicro himgoyalmicro left a comment

Choose a reason for hiding this comment

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

LGTM

@himgoyalmicro himgoyalmicro merged commit b596328 into dotnet:main Mar 6, 2025
8 checks passed
@himgoyalmicro
Copy link
Contributor

Thank you, @h3xds1nz, for your consistent and valuable contributions. Your hard work is greatly appreciated! 😄

@h3xds1nz
Copy link
Member Author
h3xds1nz commented Mar 6, 2025

@himgoyalmicro Thank you as always :)

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

@himgoyalmicro himgoyalmicro himgoyalmicro approved these changes

@dipeshmsft dipeshmsft Awaiting requested review from dipeshmsft

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

Assignees

@h3xds1nz h3xds1nz

@himgoyalmicro himgoyalmicro

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.

SizeLimitedCache should use TryGetValue
3 participants
@h3xds1nz @ThomasGoulet73 @himgoyalmicro
0