Optimize working with EllipseGeometry/RectangleGeometry, reduce allocs

[h3xds1nz]

Description

Removes some unnecessary gen0 allocations when working with EllipseGeometry/RectangleGeometry (such as adding them to a PathGeometry or combining geometries which serializes them (Point array alloc) and when rendering TickBar. Also introduces internal constructors for PathFigure/PathGeometry that can be used with inline arrays.

Also changes some private static fields to constants, decreasing static allocations.

The savings are not that massive right now, but it will allow for further optimizations (and there's a lot of space for it).

Define geometry, add to group, get bounds

Method	Mean [ns]	Error [ns]	StdDev [ns]	Gen0	Code Size [B]	Allocated [B]
Original	9,371.1 ns	67.37 ns	56.25 ns	0.4578	9,483 B	7848 B
PR_EDIT	8,986.0 ns	55.25 ns	48.98 ns	0.4120	9,120 B	7120 B

Benchmark code

public Rect Original()
{
  PathGeometry abcd =  new PathGeometry();
  RectangleGeometry rectangle =   new RectangleGeometry(s_myRect, 10, 10);
  EllipseGeometry ellipse = new EllipseGeometry(s_myPoint, 10, 10);

  // uses GetPathFigureCollection
  abcd.AddGeometry(rectangle);
  abcd.AddGeometry(ellipse);

  // Retrieve bounds, another alloc
  Rect myBounds = rectangle.Bounds;
  Rect myEllipse = ellipse.Bounds;

  myBounds.Union(myEllipse);

  return myBounds;
}

Customer Impact

Improved performance, decreased allocations.

Regression

No.

Testing

Local build, sample apps, usage of types.

Risk

Should be low, all the changes were done with using stack-allocated memory

Microsoft Reviewers: Open in CodeFlow

[robert-abeo]

@h3xds1nz What are you doing with all these micro optimization PRs?

1. Spamming this many PRs without discussion first seems like a bad idea
2. I highly double the WPF team even has the time to review these
3. Changing this amount of code with very very limited performance benefit does NOT pass the cost/benefit case study. You risk breaking things (all production apps) for almost no benefit.
4. You are introducing your own code formatting style which violates existing code and Microsoft's standard code format rules

I hope you have discussed all this with Microsoft ahead of time because it looks very strange from my viewpoint. If you want to focus on performance, great! But DO NOT spam a bunch of micro optimization PRs that risk breaking already working code. Focus on the bigger gains.

[h3xds1nz]

@robert-abeo I wasn't originally even gonna respond, but here you go.

1. The team is free to close my PRs if they deem them unfit, I'm sure they do not need your permission.

2. There's a saying you probably are not familiar with (this time, not your fault); it goes roughly as "he opened a restaurant but people started coming in". It's a nice but a very true oxymoron, much of like what you've said here.

3. You must be the reason x86_64 architecture still starts up in real mode. Every bug fix is a risk, every upstream level change (JIT, BCL) risks breaking all downstream consumers (e.g production apps) for, as you say, no benefit. Why even introduce new types, why even introduce ref structs, ability to stackalloc memory, and actually using those new features under the hood? I'd start at the core, that's dotnet/runtime. For some reason, they wanted NET unlike NetFX to be performance driven. You should go ahead and create an issue to tell everyone how you feel about the fact that about 70% of the BCL has already been rewritten using new language and runtime features for often low change-to-change benefit but overall great results, since most changes are often a building block for the next one.
   Most optimizations are incremental, sometimes you need to shave off 1 kB out of 8 kB so then you can get additional 5 kB down and 60% perf improvement in the next; not everytime you get x7 perf improvement and 10% of previously allocated memory the first time around. But why do I explain it to you? Besides everyone else's, I'm just breaking the platform my company builds apps on and about 40% of the infrastructure depends on.

4. I've resolved your conversation because it has made zero sense. The WPF codebase has obviously had changing style over the years, some parts are written with pure C/C++ conventions (member names, function names, etc.) even though they do not incorporate or relate to any unmanaged calls simply because developers writing them were used to ATL/MFC code style, some parts are generated with that same conventions; some follow early NetFX style (namings etc.), then there are parts written after the release of C# 3.0 that incorporate var (there are few but there are some) which is generally discouraged (nobody wants to think about JavaScript), and so much more variations that I could spend days on.

There are sometimes static functions that start with _ (hello C-style, I'd like you see you write this in a .NET project); members that have m_ prefix; there are constants that have c_ prefix, there are constants that have _ prefix, and then there are finally constants in PascalCase as it is recommended by the guidelines these days.

But guess what, shocker, most of the time, the conventions vary not just in a namespace, but in a single file, and in the particular file (EllipseGeometry.cs) where you've highligted the line 72; I'd recommend checking line 360; 205; 157 (original lines, those were not edited since that part of WPF was opensourced), and tell me, please, what is the coding style? Why are those function definitions as long as the particular ctor I've auto-formatted (yes, an analyzer, imagine that). Guess opposite of so called coding style in one file. If anything, I've just unified it.

If we wanna have a look at so called "Microsoft's standard code format rules", guess those would be mostly incorporated in standard tools such as Visual Studio and Roslyn analyzers, correct? You obviously weren't thinking about incorporating random Rider rules and line breaks for 1024x768 CRT monitors, calling them current "Microsoft's standard code format rules"? Plus there's of course runtime coding style which I can't really seem to have violated. https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md

Not even in C code bases you'd see auto formatters mostly to wrap 1 per line. Plus, as of like 4 years ago, there's --ignore-revs-file with with git blame where you put those 2-3 big auto format commits and nobody gets hurt. So, for example:

• I've renamed constants (where some were defined fields and some const), both prefixed with c_ as PascalCase
  • That would fall under IDE1006
• I've used pattern matching, that would be IDE0019
• I've inlined out variables, that would be C# 7? and IDE0018
• I've simplified some object inits into new() on right-hand side, that would be IDE0090
• I've simplified PathGeometryData init, that was IDE0017
• I've renamed some data types (e.g. UInt32) to language keywords, that's IDE0049
• I've simplified some collection initializations so that would be IDE0300
  • Aas that's the only way to get Roslyn to emit InlineArray parameters, not because I'm collection expressions fan
• I've changed compile-time constants that are defined as "static private byte" (not even readonly as it wasn't available at the time of writing) into actual constant definitions (const).
  • There is no compile-time checking without readonly so anyone could overwrite it in the future
  • It consumes unnecessary memory (perf opt)
  • JIT cannot treat them as constants
  • static first is mostly C-style, not really the newest C#
• I've changed Point * name (don't know what this is) and Point *name (C-Style) to Point* name - VS auto format
• I've added whitespaces between numbers on multiplication and between parentheses and code blocks
  • e.g. fixed(byte - VS auto format
• At line 384 and 390 (original lines) in RectangleGeometry, what is the coding style? Confused. Same issue as you highlighted above with EllipseGeometry. One is that way, second time it's the other way. This time it's an if condition.

And yes, it is absolutely fine to put parameters onto the next line when a single one gets too long!

Lastly, given the time and effort you must have put into your two comments, you could have put up PRs fixing half of your Fluent-styling issues instead and attempt to help everyone using WPF's new Fluent theme for a much better experience incorporating it into their applications. I'd even give you a thumbs up as it shows effort! But I understand, it is always easier to randomly criticise from distance. Please, instead of responding to my reaction here, I'd recommend creating a discussion, name it "Performance optimizations PRs should be banned as the team has not been 100% focused on the issues I have created" if you want and have a go at it. But I shall not take this from a drive-by shooter. Fixing community-reported bugs and alongside doing some optimizations does not make me the ultimate authority but it surely gives me more ground to have an educated opinion on this, and given your inability to even go through one file before saying how I've violated the codestyle that's violated 15 times in that file to begin with, it definitely doesn't hold a high value to me as a constructive feedback.

I couldn't be more constructive, simply because, well, I "focus on bigger gains" but without preparing the ground at the down level, with PRs like these, you can never get there. One last example, let's see if you can comprehend it: Span<T>.

With that being said, I'll continue on "spamming" when I feel like it unless the team's position is to close my PRs, thank you. There's 1.2k open issues and for about 80% of what I'm submitting it is contributing to fixing those issues but since it is only partially as the fixes require ground preparation often times, it does not justify slapping "Fixes #" on it.

[ThomasGoulet73]

I agree with some of what @robert-abeo said, though I really disagree with the tone used.

I'll open an issue to discuss revamping contributor guidelines but I'll leave some thoughts here:

For now, I'll say that I'm in favor of the performance enhancement in this PR but I'm against the unrelated changes around it. In my experience, this PR would have been refused in a lot of other repos (And probably in dotnet/runtime and WPF follows most of the guidelines of dotnet/runtime). Here's some things I noticed:

1. There's "+233 -488" of diff for a PR that could have ~40-50 lines of diff to achieve the same performance result.
2. Just in the first file, there is 125 lines of diff of unrelated changes: https://github.com/dotnet/wpf/pull/9981/files#diff-89ae88ad0a25ce6473bd237409a759def3e5a3c5386f30152befaec9fd0b5d6eL5-R130. IMO that's way beyond "reasonable" refactoring for a performance PR.
3. This code is not against the guidelines so why rename the variable to your prefered name ? I personally use "pXYZ", am I supposed to change it back from "ptrXYZ" to "pXYZ" if I want to modify this code in 6 months ?

The last bullet point here says "DO NOT submit pure formatting/typo changes to code that has not been modified otherwise." so technically some of the code in this PR is against the guideline.

This is a very old and very big code base, you'll find code that you would've written differently than what was written 10 years ago and you'll find code in the future that you would've written differently than what was written today.

I'm not trying to discourage you from contributing since your PRs are valuable, I'm just trying to make sure they are done in the best way possible.

[miloush]

I criticized other people for mixing actual changes and formatting changes in the past, in much smaller PRs, because it makes it difficult to review (I particularly remember one that could have been single change but was a few extra lines with formatting changes, with which the team agreed and asked the author to only do the functional change, though I cannot seem to find that PR now), so it sounds fair to uphold that view: Yes, I would certainly prefer if we could keep styling changes to minimum, preferably matching the code around. The repercussions from my side, however, is probably just unwillingness to spend time reviewing PRs (which otherwise tend to be straightforward in case of @h3xds1nz).

But I also agree that the team is free to do what they see fit. So far these PRs seem to be getting merged. Moreover, it was indicated that performance contributions are now a priority: #9920.

I wouldn't want to lose contributors like @h3xds1nz by making what they do not enjoyable, so I hope we can find some balance between what the team is willing to accept and authors to produce while allowing reviewers to help out.

[robert-abeo]

A few more points.

1. My tone is perhaps sharper than intended but take it more as shock (at what's going on and the amount of PRs like this being opened) than pure criticism.
2. Code formatting is best left for code formatting tools. If the WPF repo wants to improve code formatting (make it more consistent, etc.) or variable naming it should be done in an automated way and all at once. Then added to the git blame ignore revs.
3. In a code base this old and so widely used everyone MUST prioritize correctness. We can't risk stability at this point with existing apps. That means do not change things that don't need to be changed because every change is a risk. Some might disagree with that and I understand why but it's Microsoft's policy for the most part. The time for lots of little polishing was long ago IMO.

[rampaa]

That means do not change things that don't need to be changed because every change is a risk.

This is such an unhelpful comment. If you have concerns about specific code changes and their ramifications, by all means, please name them, that's actually very helpful for everyone involved. But if you don't actually have anything to contribute, maybe refrain from discouraging people who actually contribute from contributing. Thankfully, you don't seem to be in a position to decide (micro) optimizations are not needed, and people who actually get to decide those things have been merging those changes, that probably should have been your cue about the fact that your view about micro optimizations is not shared by others, especially by those who actually get to decide those things.

Some might disagree with that and I understand why but it's Microsoft's policy for the most part.

"Every change involves some risks, so we will avoid micro optimizations" is certainly not a Microsoft policy. 200+ pages long Performance Improvements in .NET X posts are a good testament to that. AFAICS, squeezing every bit of performance where it's possible has been the general direction of .NET.

[h3xds1nz]

Just very recently for example; #9882 - nobody said a thing. All those null-conditional access operators instead of if-checks, changes to the property styles, all it could have been was removing Value and new XYZ. Hell, even reviews from WinForms guys requested to change props/defs to a certain style as nitpicks! Per Jeremy's words

lonitra's suggestions are nits that I'll take care of in the other PR's I'm working on

Guess we're gonna see some stylistic changes from Jeremy in the parts he touches.

Some picks from the PR but there's about 400 of unrelated style changes (null-conditional is the same IL):

https://github.com/dotnet/wpf/pull/9882/files#diff-da7ede7693924ac2f3fa74ffddce4e6980bae053a6b5efccde060f46a26d1d57L1680

https://github.com/dotnet/wpf/pull/9882/files#diff-caf817454a7cc2236a8974f6210284b97dd22b57faee7dc4bf40d69433d0f7c3L2984-L2993

@ThomasGoulet73 Have you been to Winforms repo?

https://github.com/dotnet/winforms/pull/11852/files
https://github.com/dotnet/winforms/pull/11843/files
https://github.com/dotnet/winforms/pull/11651/files
https://github.com/dotnet/winforms/pull/10944/files (This one is particularly juicy in style to changes ratio!)

And there's around hundreds of PRs that are just "simplify"; because format has already been done. Terrifying, indeed.
https://github.com/dotnet/winforms/pull/11050/files
https://github.com/dotnet/winforms/pull/11044/files
https://github.com/dotnet/winforms/pull/11030/files

There's one fundamental differences between runtime - WF (or even aspnetcore) and WPF, that is; WPF ain't formatted, like at all. It didn't get any global polishing unlike other repos. You have statements that are in the middle of the screen and then 1 line put fully to the left. No consistency. So yes, it's super easy to "make" those changes.

The last bullet point here says "DO NOT submit pure formatting/typo changes to code that has not been modified otherwise." so technically some of the code in this PR is against the guideline.

Well it's easy to comfort that rule because when you press auto format, nothing changes, since the codebase has been consistently formatted, isn't it. And there aren't 200 suggestions on how to change the code with newer features (that you're accustomed to writing most of the time) because they're incorporated already.

I don't do this part because I enjoy it; I do it in smaller files so I can orient in the code. And I cannot stress how many times I just press dotnet format (because I'm used to having it accustomed to the code base) and I get as many changes as there are lines in the file and then I work through history to revert it if I don't realize it. Just the other day, I was doing some stuff around simple lines:

https://github.com/dotnet/wpf/blob/cbbd92cde6a3c1103ab71fe5c453ec8e017bfb7d/src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/TextFormatting/SimpleTextLine.cs#L88-447

It takes 300 lines for 4 functions, the function Create can be written within 70 lines with the comments, using standard conventions, with brackets, while it currently takes 160 lines.
After format it fits into a normal 16:9 monitor with a full HD resolution (rather common) and the lines do not cross 150 characters ever (it's around 130 with the longest one, usually 120 is the sweet spot and recommended iirc), which is the recommended max line length you'd want at any circumstances.

And then you can finally understand and remember/see what it does without scrolling up and down. Writing short and oriented functions/procedures is not just about principle but also about readibility.

Oh and this function of 22 lines? Tell me what is the edge case. None actually.

https://github.com/dotnet/wpf/blob/cbbd92cde6a3c1103ab71fe5c453ec8e017bfb7d/src/Microsoft.DotNet.Wpf/src/PresentationCore/MS/internal/TextFormatting/SimpleTextLine.cs#L403-425

You bet I'd rewrite it in 10 (or 6, if you remove the braces), so you don't have to think about it:

private static void CollectTrailingSpaces(List<SimpleRun> runs, TextFormatterImp formatter, ref int trailing, ref int trailingSpaceWidth)
{
    for (int i = runs.Count - 1; i >= 0; i--)
    {
        if (!runs[i].CollectTrailingSpaces(formatter, ref trailing, ref trailingSpaceWidth))
        {
            break;
        }
    }
}

I have per the team's earlier suggestion put the changes into different commits, and simply going commit-by-commit, there's not really a difference in the reviewing experience imho, I always review it myself (but obviously that's not a clear view) - here I'm happy @ThomasGoulet73 you've suggested to use merge --squash because seeing all those raw commits in main wasn't what I was going for, haha.

@miloush the PR on your mind was #7910 I guess, I've seen it recently as I was correcting the other event args. Hugh did a lot of changes in cleaning up certain parts of System.Xaml after it was opensourced, those files are way better experience than the spaghetti code around ClrObjectRuntime is tbh.

Lastly, I'd be happy to do the same thing WF and runtime did (address formatting and newer compiler features etc. globally) to reduce noise in perf PRs if its discussed properly with the community and the team. It would make my life easier (and anyone else's contributing) as well to not have to care about those things, that's a sure bet. It would get the codebase to a much better state too.

[miloush]

It would certainly help some contributors and avoid all discussions if we introduced an .editorconfig and formatted the whole repo at once. @pchaurasia14 any thoughts on that?

I am less keen on renaming things without good reason, it makes it difficult to track things across versions and does have the potential to break applications without good reason. Obviously using reflection is explicitly not supported, but we should have a reason to break it more than a personal preference or code style.

[robert-abeo]

Your responses are extremely long but don't seem to acknowledge even an understanding of my points. I'll avoid any more personal comments. So I'm just going to say this is a fundamental difference of opinion and we won't be able to resolve it. I stand by my comments that I would not approach these PRs anything like what you are doing for the reasons stated.

Microsoft historically has been very careful of unnecessary change so as not to break anything once a product is so old and widely used. The WPF team also has very limited resources. It's up to the maintainers to continue or let go the discussion.

[h3xds1nz]

Your observation is correct, I did not react to you this time. And I shall not, @rampaa has said it for me.

[ThomasGoulet73]

I opened #9994 to continue this discussion without poluting this PR.

(@h3xds1nz, I'll respond to your comment in this issue)