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

Some dwrite wrapper fixes which help Assetto Corsa launcher to start. #5

Closed
wants to merge 3 commits into from

Conversation

rbernon
Copy link

@rbernon rbernon commented Nov 29, 2021

Last change is a bit brutal but the TextStore.TypicalCharactersPerLine * 0.25 part is causing a huge number of calls to Itemize, and then to dwrite text analyzer, with a very small amount of characters, which in turn calls back to TextAnalyzerSink managed code, and the whole thing takes ages to complete (either the callback to managed code or just the number of analyzer calls that are issued).

I'm not sure what all this is doing anyway but I don't see the point of analyzing very small blocks vs analyzing a good portion of text at once.

Note that although the launcher then opens its window it still doesn't work because of CEF issues.

@rbernon
Copy link
Author

rbernon commented Nov 29, 2021

Note also that the launcher fails on another mono issue related to System.Globalization.CultureInfo being readonly. I believe it hardcodes some native fields and fails to find them, and later crashes when trying to modify the default culture info. I saw some exceptions about missing s_userDefaultCulture field, which seems to be present in mono reference sources, and I suspect the proper fix will be to make mono implementation more compatible with native.

@nsivov
Copy link

nsivov commented Nov 29, 2021

The part you removed looks very specific, it's not something you normally do, when using directwrite. Normally you'd pass as much text at once as you possibly can, so shaping and direction changes could work properly. Resulting items will have uniform {direction, script} pairs, later this is split again to have uniform {direction, script, font}. Such segments is what gets shaped with GetGlyphs(). GetGlyphs() is meant to handle surrogates, combining marks, ignorable control characters and script shaping.

It seems dangerous to remove this logic from wpf, if that's what it does on Windows.

@rbernon
Copy link
Author

rbernon commented Nov 29, 2021

Well, although I agree that it seems dangerous, it also looks like this code was just an optimization, to avoid calling into dwrite on a unnecessarily large text chunks, and implement some sort of early chunking with ad-hoc analysis for splitting at the right position.

In the launcher case, TextStore.TypicalCharactersPerLine is 100, which doesn't feel completely wrong, but that causes dwrite text analyzer calls to be only 25 chars long, which then feel very small and unnecessary.

I wonder where the performance loss exactly comes from, as I think native WPF was written in managed C++ the callbacks would still be managed as our C# implementation, and the performance would be comparable. Unless there's some actual meaningful difference between Mono and .NET in the native-to-managed overhead.

@madewokherd
Copy link
Owner

madewokherd commented Dec 6, 2021

Well, in theory, even though the bit removed by patch 1 was meant as an optimization, it is a behavior change, since TextRun implementations can be provided by the application. It's unlikely that anything depends on this specific behavior, but not impossible. (Also, it's unlikely that we can replicate the exact calls into TextRun implementations anyway.)

I would be curious how .NET behaves in this situation - is the performance issue specific to Mono (including our rewritten DirectWriteForwarder and TextFormatting) or to Wine's dwrite? There have been some performance issues observed with DirectWriteForwarder on .NET Core: dotnet#4768

It's not even clear what case this was supposed to solve. It might have been that they were seeing problems with extremely long lines and applied a solution that wasn't helpful in the vast majority of cases.

Since Wine's dwrite never calls GetNumberSubstitution at all, doing it lazily obviously improves performance for now. I guess if dwrite starts using this, and it becomes a problem again, we can probably cache them. There should be very few combinations of arguments used to create these.

Patch 3 could use a commit message describing what it actually does.

I think I'm OK with all of this, assuming it passes the few test cases we have. I'll probably rewrite the patch 3's commit message.

@madewokherd
Copy link
Owner

It seems the wpf tests regressed, so I'm going to have to figure that out first.

@rbernon
Copy link
Author

rbernon commented Dec 6, 2021

Well, in theory, even though the bit removed by patch 1 was meant as an optimization, it is a behavior change, since TextRun implementations can be provided by the application. It's unlikely that anything depends on this specific behavior, but not impossible. (Also, it's unlikely that we can replicate the exact calls into TextRun implementations anyway.)

You probably mean patch 3 (the one in PresentationCore)?

I would be curious how .NET behaves in this situation - is the performance issue specific to Mono (including our rewritten DirectWriteForwarder and TextFormatting) or to Wine's dwrite? There have been some performance issues observed with DirectWriteForwarder on .NET Core: dotnet#4768

As far as I could see for some reason it wasn't busy at all, so the slowness is either coming from some locking and waiting, or from the application side, but although this caused a lot of dwrite calls, it wasn't a dwrite performance problem. I couldn't really pinpoint precisely the issue, but I suspect it was possibly related to ping-ponging between managed and native, as the text analyzer sink callbacks are in managed code, before returning to dwrite and back to managed application code.

@rbernon
Copy link
Author

rbernon commented Dec 6, 2021

I'm not sure how to measure that, but it wasn't busy CPU-wise, in the sense that top, perf and other load measuring tool were only showing reasonably idle processes.

@madewokherd
Copy link
Owner

I wouldn't expect creating/destroying a lot of COM objects to matter unless it's to do with the GC somehow.

@madewokherd
Copy link
Owner

Thanks, merged manually.

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

Successfully merging this pull request may close these issues.

3 participants