Skip to content
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.

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

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

Open
deeprobin opened this issue Sep 13, 2021 · 43 comments
Labels
Community Discussion Performance Performance related issue
Milestone

Comments

@deeprobin
Copy link
Contributor

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
Copy link

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
Copy link
Contributor Author

deeprobin commented Sep 14, 2021

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

@madewokherd
Copy link

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
Copy link
Contributor

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:

  • Trimming support. The .Net Trimmer does not support C++/CLI assembly analysis so it might remove code that is used in DirectWriteForwarder which can produce exceptions that are hard to diagnose (Eg. .net 5 trimming causes crash on copying text #3903).
  • NativeAOT support. NativeAOT does not support C++/CLI assemblies at all, and if I remember correctly, the team responsible for NativeAOT does not plan on adding support for it. (COM interop support runtimelab#306)
  • Crossgen support (This one I'm not 100% sure). I think Crossgen does not support C++/CLI assemblies so we may be leaving performance on the table here.
  • Ease of development. I think most people in the community that would like to contribute on WPF are better in C# than C++ because I think they all used C# and/or WPF before wanting to contribute here (I might be totally wrong and this statement is not in any way data-driven). Also, most of the code in WPF is already in C# so this would be good. The ease of development might also help optimizing the C# code after the port.
  • It's a legacy technology (I think, I wasn't able to find an official statement but I remember reading it somewhere).
  • Probably others that I couldn't think of off the top of my head.

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor Author

deeprobin commented Sep 19, 2021

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

@vishalmsft vishalmsft added Community Discussion Performance Performance related issue labels Sep 21, 2021
@deeprobin
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@ThomasGoulet73
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

@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
Copy link
Contributor Author

@ThomasGoulet73 Great. Thanks a lot :)

@vatsan-madhavan
Copy link
Member

@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
Copy link
Contributor

@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
Copy link

rickbrew commented Feb 9, 2022

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
Copy link
Member

tannergooding commented Feb 9, 2022

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
Copy link
Contributor

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):

  • Write it by hand. Cons: Might make mistakes or use slower code.
  • Use CsWin32. Cons: Still in beta.
  • Use only required files from TerraFX. Cons: Dependency on third-party code.

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
Copy link
Member

tannergooding commented Feb 9, 2022

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
Copy link

rickbrew commented Feb 9, 2022

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

@ThomasGoulet73
Copy link
Contributor

@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
Copy link
Member

lindexi commented Feb 11, 2022

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

@kant2002
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

kant2002 commented Jun 7, 2022

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
Copy link
Contributor

ThomasGoulet73 commented Jun 7, 2022

@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
Copy link
Contributor

kant2002 commented Jun 7, 2022

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
Copy link
Contributor

kant2002 commented Jun 7, 2022

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 dotnet/runtimelab#1100
If somebody would be curious to try it would be better informed about roadblocks

@ThomasGoulet73
Copy link
Contributor

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
Copy link
Contributor

kant2002 commented Jun 7, 2022

Next offender is

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

@kant2002
Copy link
Contributor

kant2002 commented Jun 7, 2022

image

Only change which I do not place on the aforementioned branch is removing content of

// To avoid injection of derived System.Types that lie about their identity
// (and spoof other types), only allow RuntimeTypes.
// S.W.M.XamlReader only supports live reflection, anyway.
Type runtimeType = typeof(object).GetType();
if (!runtimeType.IsAssignableFrom(type.GetType()))
{
throw new ArgumentException(SR.Get(SRID.RuntimeTypeRequired, type), "type");
}

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
Copy link

ccarlo88 commented Sep 6, 2022

Can someone at Microsoft consider the outstanding job done by @kant2002 on here kant2002/WinFormsComInterop#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Discussion Performance Performance related issue
Projects
None yet
Development

No branches or pull requests