Skip to content

Re-merge DirectWriteForwarder into PresentationCore #1727

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

Closed
wants to merge 35 commits into from

Conversation

rladuca
Copy link
Member

@rladuca rladuca commented Aug 26, 2019

Addresses #1549

Reverting to using a NetModule and merging with a C++/CLI object file in order to build the C++/CLI and C# components of PresentationCore into a single module.

Tested:

  • DRTs have passed on x86 and x64
  • Some basic performance testing applications run without issue
  • Performance is about the same as prior tests with Preview1 binaries and hand-merged IL.
  • DRTs have passed on x86 and x64 crossgen assemblies
  • Microsuite has passed on crossgen assemblies on RS5. Full pass still in progress.
  • Full test pass has passed on crossgen assemblies across our standard OS installs.
  • Crossgen team has said there should be no issue with crossgen and the merged assembly

In Progress:

  • IL diffs of various sorts (Merged vs Unmerged vs .NET Framework Merge)

rladuca added 23 commits August 25, 2019 17:19
- Some file re-arrangement
- Adding over-arching merge project
- Updating PCoreCSharp to use PCoreCPP

  Author:    Rob LaDuca <[email protected]>
Disabling some ancillary things to focus on getting the assembly linking.
…embly info generation for interstital PCore assemblies.
… merged assembly to get appropriate assembly info.
…re can access them without the need for a resources DLL.
@ghost ghost requested review from vatsan-madhavan, ryalanms and stevenbrix August 26, 2019 16:17
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Aug 26, 2019
@ghost ghost requested a review from SamBent August 26, 2019 16:17
@rladuca rladuca self-assigned this Aug 26, 2019
@rladuca rladuca added the Performance Performance related issue label Aug 26, 2019
@vatsan-madhavan
Copy link
Member

Can we just retain the name DrectWriteForwarder and not go back to the old nomenclature of PresentationCoreCpp etc.?

@vatsan-madhavan
Copy link
Member

DirectWriteForwarder\main.cpp: We should validate under a debugger that this stuff really works.


<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PBT was failing in various ways and, at the time, it was easier to move these to base than to figure that piece out since it was functionally equivalent at runtime.

Since we have confidence in the rest of the build at this point, I can take another look at this and see if I can puzzle out what was causing issues in PBT. It's possible that they don't even occur anymore after some of the other commits.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we are getting is:

..\WindowsBase\Fonts\GlobalMonospace.CompositeFont : fatal error LNK1107: invalid or corrupt file: cannot read at 0x65B8 [d:\repos\wpf_github\src\Microsoft.DotNet.Wpf\src\ PresentationCore\PresentationCore.vcxproj]

It looks like adding the Resource here is making the linker attempt to link this into the final assembly. One reason for this might be we don't actually have a compile pass for the merge project. It's only the linker. The resources aren't carried forward apart from what the metadatamerge does, which doesn't include resources (as is evidenced by the PCoreCSharp resources not being merged automatically).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried various ways of getting this in as a straight embedded resource, but I haven't been able to do so. This is the same set of issues that caused me to create PresentationCore-CommonResources in the first place. I'd rather not do this again and putting these into WindowsBase seems like the better fix.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just remembered why you can't do this - PBT doesn't support C++/CLI.

PBT definitely doesn't support codegen for anything besides VB and C#.
I don't remember now whether we can get resource embedding to work for C++/CLI (i.e., .vcxproj) projects.

You should just do what you were doing. Sorry about randomizing you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WinFx.targets understands knows how to hook correctly into C#/VB targets - those same hooks may not work correctly when building C++/CLI projects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah all good, it made sense to look at this again because I know I just leap-frogged it when I hit it. But that all makes sense wrt to C++/CLI.

@rladuca
Copy link
Member Author

rladuca commented Aug 27, 2019

Can we just retain the name DrectWriteForwarder and not go back to the old nomenclature of PresentationCoreCpp etc.?

Yeah that's not a bad idea. PCoreCPP isn't a very good name, I'll swap it back.

@rladuca
Copy link
Member Author

rladuca commented Aug 27, 2019

DirectWriteForwarder\main.cpp: We should validate under a debugger that this stuff really works.

I already checked in WinDbg. Just did an "sxe ld dwrite" and checked that the stack runs through the PresentationCore module initializer, etc. All seemed good to me.

@vatsan-madhavan
Copy link
Member

Fix comment:

tools\Wpf.Cpp.targets:299:    that are used in WPF's C++/CLI projects. There are only two such projects - DirectWriteForwarder

@stevenbrix
Copy link
Contributor

I see the AvTraceMessages.txt file in your PR. It says the the file was renamed without changes, so it looks like is wasn't deleted? It's no longer used, that part of codegen was converted to AvTraceMessages.xml.

@rladuca
Copy link
Member Author

rladuca commented Aug 27, 2019

I see the AvTraceMessages.txt file in your PR. It says the the file was renamed without changes, so it looks like is wasn't deleted? It's no longer used, that part of codegen was converted to AvTraceMessages.xml.

That had just been sitting around, so I kept it from wherever it lived. If it's definitely no longer needed I can delete it.

@stevenbrix
Copy link
Contributor

I see the AvTraceMessages.txt file in your PR. It says the the file was renamed without changes, so it looks like is wasn't deleted? It's no longer used, that part of codegen was converted to AvTraceMessages.xml.

That had just been sitting around, so I kept it from wherever it lived. If it's definitely no longer needed I can delete it.

Yeah it isn't needed anymore.

@stevenbrix
Copy link
Contributor

stevenbrix commented Aug 27, 2019

Do we need the module initializer for DirectWriteForwarder anymore? If I remember correctly, that was only needed because we had split from netmodule?

@rladuca
Copy link
Member Author

rladuca commented Aug 27, 2019

Do we need the module initializer for DirectWriteForwarder anymore? If I remember correctly, that was only added because we had split from netmodule?

Yes, the module initializer has always been there in DIrectWriteForwarder. What we added was one in PresentationCore to call through to the DirectWriteForwarder one. Now PresentationCore returns to status quo w.r.t. to module initialization. No more clunky il round tripping.

@stevenbrix
Copy link
Contributor

Do we need the module initializer for DirectWriteForwarder anymore? If I remember correctly, that was only added because we had split from netmodule?

Yes, the module initializer has always been there in DIrectWriteForwarder. What we added was one in PresentationCore to call through to the DirectWriteForwarder one. Now PresentationCore returns to status quo w.r.t. to module initialization. No more clunky il round tripping.

Sorry, yeah I meant the one in PresentationCore! I don't see any of those files changed, unless I'm missing them?

@rladuca
Copy link
Member Author

rladuca commented Aug 28, 2019

Do we need the module initializer for DirectWriteForwarder anymore? If I remember correctly, that was only added because we had split from netmodule?

Yes, the module initializer has always been there in DIrectWriteForwarder. What we added was one in PresentationCore to call through to the DirectWriteForwarder one. Now PresentationCore returns to status quo w.r.t. to module initialization. No more clunky il round tripping.

Sorry, yeah I meant the one in PresentationCore! I don't see any of those files changed, unless I'm missing them?

PresentationCore has always had a module initializer. When I say this, I mean back in .NET Framework. The module initializer comes from the CPP portion (now called DirectWriteForwarder) since C++/CLI has direct support for a DLL entry function that becomes an initializer. When this was split, we added back an initializer in PresentationCore (that was now fully C#) via IL modification (as C# does not support module initialization directly) so as to call through to DirectWriteForwarder and preserve initialization order. Since we're going back to the old way of doing things, the IL(D)Asm technique is gone, and we just get the module initializer for free from the merged C++/CLI portion (DirectWriteForwarder). So, still have one, still need one, just doing it the way it was done historically prior to the split.

@rladuca
Copy link
Member Author

rladuca commented Aug 29, 2019

We have a better pathway to fix this, closing. See #1744

@rladuca rladuca closed this Aug 29, 2019
@weltkante
Copy link

Thanks anyways for doing this in the open, I appreciate having an example of how to build and merge netmodules, this is pretty underdocumented. I've had to do this in the past and it was a lot of trial and error and I don't think I ever got it entirely right. I'm preserving the branch for future reference, if thats ok with you.

@rladuca
Copy link
Member Author

rladuca commented Aug 29, 2019

Thanks anyways for doing this in the open, I appreciate having an example of how to build and merge netmodules, this is pretty underdocumented. I've had to do this in the past and it was a lot of trial and error and I don't think I ever got it entirely right. I'm preserving the branch for future reference, if thats ok with you.

Preserving it is completely fine. Just to make sure you realize, currently the private C++/CLI tools we use are necessary to build this merged module. Otherwise you will hit a linker error during metadata merging of the individual modules. These changes will eventually make their way to preview/public versions of VS, but they are not there just yet.

There are other uses for this as well within the WPF build and we may yet come back to some version of this to accomplish other things.

If you have any questions about parts of this, feel free to ask!

@vatsan-madhavan vatsan-madhavan deleted the dev/roladuca/remergepcore branch September 16, 2019 22:16
@ghost ghost locked as resolved and limited conversation to collaborators Apr 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Performance Performance related issue PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants