8000 Optimize parsing/conversion of TextDecorations from string, reduce allocations by h3xds1nz · Pull Request #9778 · dotnet/wpf · GitHub
[go: up one dir, main page]
Skip to content

Optimize parsing/conversion of TextDecorations from string, reduce allocations #9778

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

Optimize parsing/conversion of TextDecorations from string, reduce allocations #9778

merged 12 commits into from
May 16, 2025

Conversation

h3xds1nz
Copy link
Member
@h3xds1nz h3xds1nz commented Sep 13, 2024 •
edited
Loading

Description

Optimize parsing of TextDecorations from string into TextDecorationCollection, reduces allocations in all steps and improves performance, also reduces the memory footprint + readibility of the entire class by a large amount.

  • Removes static array TextDecorationNames, that also removes the strings allocations on its own (not used anywhere else).
  • Removes static array PredefinedTextDecorations, which was redundant from the start.
  • And removes another 120 lines trying to avoid allocations in NetFX by using few newer BCL features.
  • The static collections are frozen after creation, so are its items, so we can just grab it via indexer.

ConvertFromString benchmarks

Method Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original 252.0 ns 1.16 ns 1.08 ns 0.0234 1,243 B 392 B
PR__EDIT 119.0 ns 0.41 ns 0.38 ns 0.0091 3,335 B 152 B 
ConvertFromString("  Strikethrough   ,Underline, Baseline        , Overline             ");
Method Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original 56.77 ns 0.544 ns 0.509 ns 0.0091 1,141 B 152 B
PR__EDIT 36.73 ns 0.414 ns 0.388 ns 0.0067 2,659 B 112 B 
ConvertFromString("Strikethrough");
Method Mean [ns] Error [ns] StdDev [ns] Gen0 Code Size [B] Allocated [B]
Original 20.19 ns 0.318 ns 0.298 ns 0.0048 1,119 B 80 B
PR__EDIT 15.81 ns 0.265 ns 0.247 ns 0.0048 1,730 B 80 B 
ConvertFromString("None");

Customer Impact

Improved performance, decreased allocations.

Regression

No.

Testing

Local build, CI, extensive assert testing:

private static void CheckEqual(TextDecorationCollection collection1, TextDecorationCollection collection2, int expectedCount)
{
    //Check count
    Assert.AreEqual(expectedCount, collection1.Count);
    Assert.AreEqual(collection1.Count, collection2.Count);

    for (int i = 0; i < collection1.Count; i++)
    {
        Assert.AreEqual(collection1[i], collection2[i]);
    }
}

// Valid cases
// "None" returns no items
CheckEqual(TextDecorationCollectionConverterNEW.ConvertFromString(string.Empty),
              TextDecorationCollectionConverter.ConvertFromString(string.Empty), 0);
CheckEqual(TextDecorationCollectionConverterNEW.ConvertFromString("          "),
              TextDecorationCollectionConverter.ConvertFromString("          "), 0);
CheckEqual(TextDecorationCollectionConverterNEW.ConvertFromString("None"),
              TextDecorationCollectionConverter.ConvertFromString("None"), 0);
CheckEqual(TextDecorationCollectionConverterNEW.ConvertFromString("    None    "),
              TextDecorationCollectionConverter.ConvertFromString("    None    "), 0);

CheckEqual(TextDecorationCollectionConverterNEW.ConvertFromString("Strikethrough"),
              TextDecorationCollectionConverter.ConvertFromString("Strikethrough"), 1);

CheckEqual(TextDecorationCollectionConverterNEW.ConvertFromString("Strikethrough       "),
              TextDecorationCollectionConverter.ConvertFromString("Strikethrough       "), 1);

CheckEqual(TextDecorationCollectionConverterNEW.ConvertFromString("Underline, Baseline "),
              TextDecorationCollectionConverter.ConvertFromString("Underline, Baseline "), 2);

CheckEqual(TextDecorationCollectionConverterNEW.ConvertFromString("  Strikethrough   ,Underline, Baseline "),
              TextDecorationCollectionConverter.ConvertFromString("  Strikethrough   ,Underline, Baseline "), 3);

CheckEqual(TextDecorationCollectionConverterNEW.ConvertFromString("  Strikethrough   ,Underline, Baseline        , Overline             "),
              TextDecorationCollectionConverter.ConvertFromString("  Strikethrough   ,Underline, Baseline        , Overline             "), 4);

// Special case

Assert.AreEqual(TextDecorationCollectionConverterNEW.ConvertFromString(null), TextDecorationCollectionConverter.ConvertFromString(null));

// Exception cases
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverterNEW.ConvertFromString(",  Strikethrough   ,Underline, Baseline "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverterNEW.ConvertFromString("  Strikethrough  , Strikethrough, ,Underline, Baseline "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverterNEW.ConvertFromString("None,  Strikethrough   ,Underline, Baseline "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverterNEW.ConvertFromString("None,  Strikethrough   ,Underline, Baseline, Overline "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverterNEW.ConvertFromString(" Strikethrough   ,Underline, Baseline, Overline, x "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverterNEW.ConvertFromString(" Strikethrough   ,Underline, Baseline, Overline, "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverterNEW.ConvertFromString(" Underline, Underline "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverterNEW.ConvertFromString(" Noneee "));

Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverter.ConvertFromString(",  Strikethrough   ,Underline, Baseline "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverter.ConvertFromString("  Strikethrough  , Strikethrough, ,Underline, Baseline "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverter.ConvertFromString("None,  Strikethrough   ,Underline, Baseline "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverter.ConvertFromString("None,  Strikethrough   ,Underline, Baseline, Overline "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverter.ConvertFromString(" Strikethrough   ,Underline, Baseline, Overline, x "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverter.ConvertFromString(" Strikethrough   ,Underline, Baseline, Overline, "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverter.ConvertFromString(" Underline, Underline "));
Assert.ThrowsException<ArgumentException>(() => TextDecorationCollectionConverter.ConvertFromString(" Noneee "));

Risk

Should be safe, I believe I've tested thoroughly all cases and we shall have no surprises.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners September 13, 2024 20:52
@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 Sep 13, 2024
@h3xds1nz h3xds1nz mentioned this pull request Oct 4, 2024
Merged
@dipeshmsft dipeshmsft self-assigned this Oct 8, 2024
@h3xds1nz h3xds1nz force-pushed the textdecorationconverter-speedup branch from 721af99 to 00c98c2 Compare January 22, 2025 15:50
@h3xds1nz h3xds1nz force-pushed the textdecorationconverter-speedup branch from 00c98c2 to 7b630e3 Compare March 24, 2025 18:15
@codecov Codecov
Copy link
codecov bot commented Mar 24, 2025 •
edited
Loading

Codecov Report

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

Project coverage is 11.21368%. Comparing base (6514196) to head (7b630e3).
Report is 122 commits behind head on main.

Additional details and impacted files 
@@                 Coverage Diff                 @@
##                main       #9778         +/-   ##
===================================================
- Coverage   11.22814%   11.21368%   -0.01446%     
===================================================
  Files           3352        3352                 
  Lines         668000      667934         -66     
  Branches       74980       74971          -9     
===================================================
- Hits           75004       74900        -104     
- Misses        591745      591871        +126     
+ Partials        1251        1163         -88
Flag Coverage Δ
Debug 11.21368% <97.43590%> (-0.01446%) ⬇️

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.

@h3xds1nz
Copy link
Member Author

@dipeshmsft Any news on this one? We've merged the tests several weeks ago already for this one.

@dipeshmsft dipeshmsft merged commit dec6626 into dotnet:main May 16, 2025
8 checks passed
@dipeshmsft
Copy link
Member

Thanks @h3xds1nz for this contribution.

@h3xds1nz
Copy link
Member Author

@dipeshmsft Great, happy to see it in :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miloush miloush miloush left review comments

@ThomasGoulet73 ThomasGoulet73 ThomasGoulet73 left review comments

@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
Projects
None yet
Milestone
No milestone
Development

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