dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.03k stars 1.16k forks source link

[Performance] Rewrite DirectWriteForwarder in C# to improve rendering performance #5305

Open deeprobin opened 3 years ago

deeprobin commented 3 years ago

See #4768.

The current code is written in C++/CLI and I think that can be optimized. Unfortunately, I have written very little C++/CLI myself but I suspect that the generated IL code is smaller and better performing.

See also https://github.com/dotnet/wpf/tree/main/src/Microsoft.DotNet.Wpf/src/DirectWriteForwarder

madewokherd commented 3 years ago

Hi, I did this last year but I needed a small C component to get around some COM interop issues. https://github.com/madewokherd/wpf/blob/main/src/Microsoft.DotNet.Wpf/src/DirectWriteForwarder/CPP/DWriteWrapper/Factory.cs

deeprobin commented 3 years ago

@madewokherd Oh cool 👍🏼 Is in this already in Pull Request?

madewokherd commented 3 years ago

No, I did it because I needed a way to build it on Linux using open source tools for wine-mono. I didn't expect it to be useful for upstream.

It probably needs to be cleaned up. The few C functions needed could be moved to an existing dll, or maybe ComWrappers would remove the need for them. There's still TrueTypeSubsetter which I didn't touch at all, it doesn't seem to be commonly used and could be moved to pure C++ I think. And the C# code probably shouldn't be mixed in with existing C++ code.

It also needs build system updates as I just used my own build system (my requirements were to build on Linux with Framework Mono, so the existing build files were not usable for my purposes).

I'm skeptical that this will improve performance. We still have to call into dwrite.dll so the native interop is just done in a different place.

Maybe I can at least put something together so you can do performance testing, once I find some free time.

ThomasGoulet73 commented 2 years ago

Hey, this is a great idea and I've actually been working in my free time on converting DirectWriteForwarder to C# for the past couple of weeks. The performance from the move from C++/CLI shouldn't be that noticeable since, as @madewokherd stated, we're still calling into dwrite.dll. But, there are other advantages, here are some in no particular order:

For now, the prototype that I'm working on is not open source because it's not working yet, but I'm making progress. I took a slightly different approach than @madewokherd, I use structs for DWrite objects that I wrap in a CriticalHandle so the lifetime management is done by the GC and we almost never have to manually release them. We don't really need ComWrapper because if I recall correctly, DWrite is not "true COM". Most of DWrite objects are created by there parent object and one-way only (DWrite -> Managed).

Keep in mind that what I said might be totally untrue but for now, what I've done seems to be working.

We would also need confirmation from someone in the WPF team that they would be interested in a community PR for this, because if it's not built-in, the usefulness would be really limited.

Thanks!

madewokherd commented 2 years ago

Apparently the part I had trouble with was implementing the IDWriteFontCollectionLoader and IDWriteFontFileLoader interfaces on the C# side. We would sometimes get calls to these interfaces after calling the corresponding IDWriteFactory::Unregister* method.

I don't remember exactly why that was a problem, but I was convinced at the time that I couldn't fix it while using builtin COM support to implement the interface, and I didn't want to rewrite it based on delegates.

Incidentally, that's the sort of reference cycle involving unmanaged objects that I don't think the GC will be able to detect for you.

ThomasGoulet73 commented 2 years ago

I didn't hit this yet so maybe my app doesn't go far enough or it's something specific to built-in COM. I'll look into it once I hit this but thanks for the info!

My current plan for the port is this:

  1. Remove reference to DirectWriteForwarder in PresentationCore.
  2. Add every class that existed DirectWriteForwarder and that are required by PresentationCore.
  3. Implement every required method as throw NotImplementedException.
  4. Start a demo app using the built assemblies and wait for NotImplementedException.
  5. Port the method from C++/CLI to C#.
  6. Start again from 4.

I'm currently doing steps 4 to 6. This might not be the most efficient way but I wanted to do it small steps at a time to make sure that every method works and that it doesn't start breaking everywhere. This is a direct port so there are no changes other than porting C++/CLI code to C#.

deeprobin commented 2 years ago

@ThomasGoulet73 Thank you for taking a look at this. If you need some assistance, I can help you 😄.

deeprobin commented 2 years ago

I have created a benchmark for DWrite in general. This benchmark covers the direct call of the methods via COM. The COM overhead cannot be read out here, of course, but perhaps it would be a future consideration to no longer use DWrite but to rewrite the complete font rendering in C#.

Benchmark on AMD Ryzen 5 3600 (6-Core): https://files.deeprobin.de/dwrite-bench/report/ Source Code: https://github.com/deeprobin/dwrite-benchmark

@ThomasGoulet73 But of course the first step is to get rid of the C++/CLI code.

ThomasGoulet73 commented 2 years ago

I have some updates, I ported a good chunk of C++/CLI code to C#, enough to run a simple app without needing any C++/CLI code. I'll start cleaning my port and I will push it to my fork when it's in a "good enough" state. There's still a large task of testing everything but from my initial testing, it seems to work.

One nice thing that I noticed is that I was now able to trim a simple WPF app with few or no linker hints/configurations.

deeprobin commented 2 years ago

@ThomasGoulet73 Oh thats nice. Thanks for the time you put into this.

ThomasGoulet73 commented 2 years ago

I pushed my changes to a branch in my fork https://github.com/ThomasGoulet73/wpf/tree/managed-dwrite for those of you who want to try it. There are few methods that are still not implemented because I didn't hit them yet.

My next step is to do some testing using WPF samples and then try to split this branch in multiple PRs to try and merge it progressively.

deeprobin commented 2 years ago

I pushed my changes to a branch in my fork https://github.com/ThomasGoulet73/wpf/tree/managed-dwrite for those of you who want to try it. There are few methods that are still not implemented because I didn't hit them yet.

My next step is to do some testing using WPF samples and then try to split this branch in multiple PRs to try and merge it progressively.

Oh that seems great. Good work!

A small thing, besides the unimplemented methods: Couldn't this theoretically be converted to a struct that doesn't use an object but an int (the only constructor call of Span is an int and by using int it is not boxed). This would allow the compiler to allocate the span on the stack. Since I don't know all the background, feel free to be critical :). Code Snippet / Span

ThomasGoulet73 commented 2 years ago

@deeprobin That's some nice suggestions but for now I'm not really looking at performance optimizations. Span could probably be generic because according to this comment, it was created before C# supported generics. I'll try to take a deeper look at your suggestions once I'm done with the full port of DirectWriteForwarder to managed.

deeprobin commented 2 years ago

@ThomasGoulet73 Great. Thanks a lot :)

vatsan-madhavan commented 2 years ago

@ThomasGoulet73 this looks like a nice effort - awesome!

Instead of defining your own P/Invoke and struct definitions, see if you can make use of CsWin32. I was recently using it for something and was able to leverage many IDWritexx COM interfaces defined by CSWin32 automatically. I was thinking that would really make DirectWriteForwarder C#-ification easier and came here to check what's going on - turns out you're already working on it 👍

ThomasGoulet73 commented 2 years ago

@vatsan-madhavan That's great, I didn't know that CsWin32 was able to generate DirectX csharp definitions. I'll try to have a look on how to integrate this on my branch though I'm hesitant to use it because it's still in beta. Having handwritten DirectWrite csharp definitions (Like on my branch) is a bit problematic because there are no guarantee that they are actually right, it would only fail at runtime.

rickbrew commented 2 years ago

If you're doing interop to DWrite in C#, I highly recommend making use of @tannergooding 's TerraFX.Interop.Windows package. It's written to be as low-overhead as possible, and will be the best performing solution (I did take a look at e.g. https://github.com/ThomasGoulet73/wpf/tree/managed-dwrite and it is using some patterns, such as liberal use of fixed within each wrapped COM method, that will not likely be good for perf). Its license is MIT, so the code (which is all generated) could be lifted and used even within WPF. (AFAICT, IANAL) I use TerraFX in Paint.NET, fwiw, and it's been exceptional.

tannergooding commented 2 years ago

Also noting that microsoft/cswin32 is driven off of microsoft/win32metadata which in turn builds its metadata using dotnet/clangsharp (which I maintain) + some post processing.

TerraFX.Interop.Windows is just using dotnet/clangsharp and skips the other bits. It also only provides the low-level unsafe/bindings and so is basically guaranteed to be as low-overhead and as 1-to-1 as you can get in .NET. It also explicitly targets .NET 6 and so you don't have to worry about inefficiencies creeping in due to things that aren't supported on .NET Standard or .NET Framework. It is likewise fully trimmable and so while the root assembly is ~17mb; with trimming most apps can get it down to between 50-400kb depending on how much of the surface area you need.

ThomasGoulet73 commented 2 years ago

Thanks @rickbrew and @tannergooding for the suggestion! I don't think we could use TerraFX.Interop.Windows in WPF as a dependency because of its size. It might be highly trimmable but WPF is not trimmable.

So it looks like there's 3 choices (In no particular order):

These 3 choices do not change the code in WPF since they all produce similar code.

For now, I'll keep writing it by hand but the other choices should definitely be considered. I'll let the WPF team make the final decision if/when the time comes of merging my branch upstream.

tannergooding commented 2 years ago

Using TerraFX.Interop.Windows in WPF probably isn't feasible (I had misunderstood and thought we were discussing a separate standalone thing); but pulling parts of it in from source or utilizing dotnet/clangsharp to only generate blittable bindings for the parts of the Windows SDK it needs should be.

rickbrew commented 2 years ago

Yeah, I meant to say to copy the code from TerraFX, not reference the package itself.

ThomasGoulet73 commented 2 years ago

@tannergooding I think I'll do what you suggested and use dotnet/clangsharp. I tried it locally and it looks very easy to generate the required bindings using dotnet/clangsharp and it's better than doing it by hand. Thank you both!

lindexi commented 2 years ago

I don't think it matters what library to choose.

kant2002 commented 2 years ago

So seems to be that’s unlock me on WPF and NativeAOT support. @ThomasGoulet73 you should expect contributions when this appear on my radar

kant2002 commented 2 years ago

Okay I take a look at the source code only. Did not have access to Windows PC right now (will test in next couple days max).

I would like to discuss following idea:

  1. I think rewrite should remove CPP DirectWriterForwarder and replace with almost identical (if not fully implemented) library. Same logic applies to System.Printing.
  2. That way we can create out-of-band package which inject into MSBuild. Even if PR with changes will sit there until full regression happens (in optimistic scenario) that package can be served to testing WPF and NativeAOT (ComWrappers mostly work)
  3. Also it is easier to integrate changes back into WPF that way, since binary contract for library would be preserved (or to be much more closer).
  4. Optionally (and unrelated to current issue) we may want to champion dependency split in the "new WPF" like WinForms did. I did not remember System.Window.Forms.Primitives in original WinForms, so that mean with enough interest we can champion that approach.

First 3 bullet points is attempt to account for situation is MS will crawl with adding support for these changes for any reason. That hedge risks without knowing what direction WPF team takes.

What do all of you thinking? Is this practical suggestions/ideas?

kant2002 commented 2 years ago

@ThomasGoulet73 when trying to build from aforementioned branch, I receive bunch of C++ errors like this

src\Microsoft.DotNet.Wpf\src\WpfGfx\core\uce\hwndtarget.cpp(697,131): error C2039: 'DisableDirtyRectangles': is not a member of 'MilRTInitialization'

Is this expected?

ThomasGoulet73 commented 2 years ago

@kant2002 I don't know, it looks a bit weird. Do you have the same error on main ? Because I haven't changed anything in WpfGfx.

ThomasGoulet73 commented 2 years ago

@kant2002 I had the same error but I fixed it by running git clean -xfd and then starting a build. DisableDirtyRectangles was added recently so there might be a cache somewhere that fails the build when pulling the main branch.

kant2002 commented 2 years ago

Yeah. I remove artifacts folder and that was enough. I successfully launch empty WPF application from your branch.

For anybody interested in NativeAOT, it is still fails, now at System.Printing initializers.

ThomasGoulet73 commented 2 years ago

@kant2002 I took a look at you're ideas but I think we should just move the code from DirectWriteForwarder into PresentationCore (Where it was in .Net Framework). By doing this, we can move code in small chunk instead of replacing DirectWriteForwarder with a new dll at once. I don't think binary contract matters here because DirectWriteForwarder is an internal only dll with its internals visible to PresentationCore and without ref assembly. This could only break if PresentationCore and DirectWriteForwarder don't have the same version at runtime which I think is very unlikely (Though I don't know the binary contract requirements for .Net).

kant2002 commented 2 years ago

@ThomasGoulet73 I lurk on repo more and agree with this decision. DirectWriteForwarder is non-public API so that make sense. System.Printing is part of contract, so we should rewrite it in one go somehow, but that's not related to that specific issue.

  1. Will you start flow code from your branch to this repo?
  2. How we plan to test that independently of WPF team?
  3. Should we maintain fork where we will make progress, and copy code here, when WPF team would be ready for review?
ThomasGoulet73 commented 2 years ago
  1. Yes I'll try to do this in the coming weeks to try to get as much changes as possible in .Net 7.
  2. Right now I only did manual testing with sample apps. There might already be text rendering in the dotnet/wpf-test repo related to text rendering that we could use.
  3. My initial plan was to keep my current branch as a baseline and extract changes in small chunks and open a PR upstream, pull the PR onto the baseline branch and test it there, rinse and repeat.

I'm happy to take suggestions.

kant2002 commented 2 years ago

I just trying to ask you opinion, so we will have coherent understaning. separate branch is fine for me. I would interested in DirectWriteForwarder rewrite because I want make WPF NativeAOT friendly, but that's bigger amount of code which would be separate.

So question do we aligned on NativeAOT goal, or I should maintain slightly separate fork and we will be kept in sync? I prefer to have more people involved in NativeAOT independently, but really you should do what's interesting to you.

rickbrew commented 2 years ago

Removing DirectWriteForwarder.dll could still be a breaking change for things like build/packaging scripts. But, it's the type of breaking change that's completely appropriate for a major release (e.g. .NET 7.0).

ThomasGoulet73 commented 2 years ago

With my changes in https://github.com/ThomasGoulet73/wpf/commits/managed-dwrite + some of @kant2002's work on ComWrappers, I am now able to compile a simple WPF app with NativeAOT. Some things still require more ComWrappers work, like text input, drag drop, etc. I also had to disable some things in the WPF codebase + do some manual NativeAOT configuration in the project but atleast it runs.

Trimming is also a big problem, you can't trim many WPF assemblies because of heavy use of reflection, like in XAML loading (It would require a XAML to C#/IL compiler). Right now, a simple app compiled with NativeAOT is ~85 mb.

I'll try to push my NativeAOT experiment on a branch in my fork soon.

kant2002 commented 2 years ago

How do you do that? I cannot compile until I almost fully rewrite DWriteForwarder (based on @ThomasGoulet73 work and continue it) and get rid of System.Printing and ReachFramework breaking compatibility for now. https://github.com/kant2002/wpf/tree/kant/no-com

More additional work, and in general I’m tired waiting, so I plan first create at least some sort of working NativeAOT experience and then rethink how this should be rearchitected.

What I learn so far: TTF parsing in native part not worth porting to C# and maybe provided using Pinvoke . That’s enough to move code in DirectWriteForwarer to C#. I order to did something with System.Printing probably first step is to break all cycles and make dependencies linear. Without that I cannot even migrate code step by step because I’m tired of chasing strange compilation errors.

TextBox still not working yet. And more interfaces should be ported to ComWrappers. I welcome anybody to join.

ThomasGoulet73 commented 2 years ago

@kant2002 I just build the WPF codebase and publish with NativeAOT using those dlls. Then I just debug the app and wait for a crash.

I couldn't get TextBox working because of the use of built-in Com interop which requires to be migrated to ComWrappers.

For now, I tried Label, Button, TextBlock, DataGrid. My tests were not thorough but basic functionality works.

When you say "I cannot compile until I almost fully rewrite DWriteForwarder" do you mean that the NativeAOT compilation fails or that the app from the NativeAOT compilation does not run ?

kant2002 commented 2 years ago

Compiled app fails but in C++ module intialization and now I see the difference. I start with TextBox and did not test other controls. TextBox dig dee into MS Text Services Framework so that’s not unexpected. So probably if you want TextBox you are going to have more code adopted. I would try other controls, thanks for the tip! Printing would be hard. Probably we should collaborate more, you can drop me a line at email on my profile, if you like to chat

kant2002 commented 2 years ago

Other important thing for visibility is to provide list of all built in controls and at least smoke check them . Similar to what I done with WinForms https://github.com/dotnet/runtimelab/issues/1100 If somebody would be curious to try it would be better informed about roadblocks

ThomasGoulet73 commented 2 years ago

I got lucky because I tried Label and TextBlock first because I was working on DirectWriteForwarder and I only tried TextBox after I was able to compile using NativeAOT.

I agree that Printing will probably be hard, I don't plan on working on it right now because I don't think that it is a priority.

Other important thing for visibility is to provide list of all built in controls and at least smoke check them

This is a good idea because some controls will definitely be more complicated to support (Like TextBox).

kant2002 commented 2 years ago

Next offender is https://github.com/dotnet/wpf/blob/144290982a74ce332d78e873cf52e3c4cb789d29/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/SystemResources.cs#L803

That's crashing on loading PresentationFramework.Aero, Any ideas how to manually preload themes from this assembly, or disable theming altogether?

kant2002 commented 2 years ago

image

Only change which I do not place on the aforementioned branch is removing content of https://github.com/dotnet/wpf/blob/144290982a74ce332d78e873cf52e3c4cb789d29/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Markup/Baml2006/WpfSharedXamlSchemaContext.cs#L50-L57

which does not work in NativeAOT for some reasons. I suspect that this is ILC bug. Application used for testing https://github.com/kant2002/WinFormsComInterop/tree/main/experiments/WpfTestBed

ccarlo88 commented 2 years ago

Can someone at Microsoft consider the outstanding job done by @kant2002 on here https://github.com/kant2002/WinFormsComInterop/issues/30 ? After that please, appoint him as CEO. The CEO the true C# developers deserve, kind of tired of the Python# developers 🤣

EDIT: @kant2002 if I could, I would give you an Oscar