Skip to content

Fix DebuggableAttribute in InjectModuleInitializer post-build targets #1745

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

Merged
merged 3 commits into from
Aug 29, 2019

Conversation

rladuca
Copy link
Member

@rladuca rladuca commented Aug 29, 2019

Addresses #1549

In the ReconstituteDll target, we call ILAsm to merge a module constructor into the assembly we previously disassembled. We need to enable JIT optimizations here so that we get an appropriate DebuggableAttribute on the assembly and don't cause perf problems.

@vatsan-madhavan who helped debug and find this fix.


Issue: #1549
PR:#1745

Description

Fix DebuggableAttribute in PresentationCore.dll

Background:

In .NET Core 3.0 Preview 6 and .NET Framework 4.x, PresentationCore was a single assembly that consisted of C# and C++/CLI components merged together by link.exe (using netmodules as inputs).

In Preview 2, PresentationCore was split into C# and C++/CLI component assemblies (PresentationCore and DirectWriteForwarder, respectively) to work around bugs in the new C++/CLI toolsets that prevented them from being merged into a single DLL effectively.

Due to this split, a module constructor implemented (#741) on the C++/CLI side was no longer running in the same initialization order with respect to the rest of WPF. This caused regressions in WPF’s high DPI support (#334)

Issue Details:

The fix for the high DPI issue (#741) introduced a regression where the DebuggableAttribute on PresentationCore was not being correctly set and was disabling JIT optimizations (by setting DebuggingModes.DisableOptimizations on PresentationCore.dll)

This is causing a 30% perf regression in warmed up repro applications and close to 50% in cold start scenarios. These regressions mainly revolve around text rendering and layout code which is the main set of code that was split off in the aforementioned Preview 2 change.

After investigation across WPF and the JIT teams, WPF was alerted to the incorrect attribute and we root caused it to a missing parameter in ILAsm (used to inject a module constructor into PresentationCore) to enable JIT optimizations.

After this change, PresentationCore will have DebuggingModes.Default | DebuggingModes.IgnoreSymbolStoreSequencePoints, which is functionally identical to all other “Release” configuration assemblies.

 ### Customer Impact

Severity:

All text rendering is at least 30% slower across the board in WPF.

We tested this via several applications that render large amounts of text via different methods:

  • Generate and render random strings into a TextBox
  • Generate a large DataGrid of strings and scroll through it many times
  • Generate a large ListView of strings and scroll through it many times

When testing without warm-up, the regression is more than 30% (50%-ish).

Applicability:

  • These are main line scenarios for WPF, esp. LoB applications.
  • Apps that show any kind of text (DataGrid, for e.g., in an LoB app) will be affected immediately.
  • Quantity of text being shown doesn’t matter – whether the text is likely to change affects performance.
    For e.g., if text is shown in a ListBox or DataGrid, and a user interacts with it to look at data, the changes in the text will immediately affect performance.

Regression?

Yes, from .NET Framework 4.8 and .NET Core 3 Preview 5

Risk

  • Low
  • Mitigations:
    • We’ve done a full test passes using the new binary
    • The change is incredibly small in scope and does not affect reconstitution of PresentationCore apart from changing the DebuggableAttribute configuration.
    • This configuration of DebuggableAttribute is generally used across all other managed WPF binaries.
  • Validations
    • We’ve verified the perf impact is no longer present when swapping in the fixed binary
    • We’ve provided self-contained published applications to customers to test the performance
    • Full test pass is completed with no issues

@rladuca rladuca added * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) area-infrastructure Performance Performance related issue ask-mode labels Aug 29, 2019
@rladuca rladuca added this to the 3.0 milestone Aug 29, 2019
@rladuca rladuca self-assigned this Aug 29, 2019
@ghost ghost requested review from vatsan-madhavan, ryalanms and stevenbrix August 29, 2019 05:30
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Aug 29, 2019
@ghost ghost requested a review from SamBent August 29, 2019 05:30
@rladuca rladuca force-pushed the dev/roladuca/ilasmfix3 branch from 4382931 to d7d3472 Compare August 29, 2019 05:32
@rladuca rladuca removed the * NO MERGE * metadata: The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 29, 2019
@rladuca
Copy link
Member Author

rladuca commented Aug 29, 2019

Approved by tactics

@rladuca rladuca merged commit 54788be into release/3.0 Aug 29, 2019
@vatsan-madhavan vatsan-madhavan deleted the dev/roladuca/ilasmfix3 branch August 29, 2019 19:15
@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
area-infrastructure ask-mode 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.

2 participants