Optimize parsing/conversion of TextDecorations from string, reduce allocations

[h3xds1nz]

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

[ThomasGoulet73]

I think it would be cleaner with 4 separate bool variables instead of a single byte variable. IMO it's overly complicated for no benefit. The original code was using the index in the array as the bit but since you changed it to separate ifs instead of the array, you can now use separate booleans.

[ThomasGoulet73]

I think you should flip this condition, do the cheaper check first:

Suggested change

	if (decoration.Equals("Overline", StringComparison.OrdinalIgnoreCase) && (matchedDecorations & OverlineMatch) == 0)
	if ((matchedDecorations & OverlineMatch) == 0 && decoration.Equals("Overline", StringComparison.OrdinalIgnoreCase))

[h3xds1nz]

Since this would be a case when we're already going to throw if it doesn't work out, the other day @miloush told me I don't need to optimize anymore; comparing the text first seemed more readable as you get it aligned under eachother.

(miloush, what is that thumbs up doing there?!)

[ThomasGoulet73]

This is my personal opinion, feel free to take it or leave it:

I've noticed in your PRs that you tend to add a lot of comments to explain what your code does, essentially duplicating the logic in the code and in the comment. These types of comment tend to make it harder to read the code, modify the code and they often become outdated when the code changes. Comments are most useful when explaining why the code does what it does, adding a reference to something (Like an issue or web link) or explaining something that is not easily understandable by reading the code.

If you want to read more about this, I would suggest this blog post which goes into more details: https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/

Your PR doesn't seem to go against any of the coding guidelines so at the end of the day it's up to the WPF team to decide if you need to change anything in your PR.

Again, this is my personal opinion and I won't pretend I'm the absolute authority when it comes to code comments. This is merely a suggestion since I don't know your background and I don't know if you might find this suggestion useful.

[h3xds1nz]

Thanks Thomas, I agree this one for example isn't that relevant and just duplicates the whole proc.

Working with large codebases especially written in C and some still in assembly, same as largely working with disassembly (not everyone gets the beauty of working with VS, the debugger its tooling), you start to comment way more even the more obvious things to follow the train of thought.

Often times, when fixing bugs, debugging, few years after someone, you'll find that the most important thing is not the code written, but the intention behind one programmer's code (which often is just a comment, since they left). People make mistakes, today's CRs and collaboration over one's code are way more prevalent than they used to be (at least in my experience), and when you don't have that clue, it is harder to debug and fix such code should there be a bug. Does maintability in case of any refactoring hurt? Yes, you have to take care of the comment. Does it help you to make more informed decision? It does.

Then again, we're in .NET when most things are way more expressive and it is easier to go through code itself and understand the intent because multiple things are abstracted away, so less comments like this are needed. When I do these refactorings on whole class, I just write down comments on stuff I shouldn't forget and then just run with it, and yes, sometimes I do not delete them ("add" them instead).

So it is indeed something I could improve on when swapping to .NET each time. Habits die hard.