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

Remove WindowsGraphics and DeviceContext #3553

Merged
merged 7 commits into from Jul 18, 2020

Conversation

JeremyKuhne
Copy link
Member

@JeremyKuhne JeremyKuhne commented Jul 10, 2020

Resolves #3570

This change removes all of the misc\GDI classes and replaces them with new scopes and isolated caches for Font-to-HFONT and "measurement" screen DCs.

This reduces significant allocations and complexity and improves performance with the related code paths. It also facilitates continued unravelling of HDC Graphics wrapping.

This is the first draft PR. It works and passes tests, with the exception of Maui for some reason which isn't clear. I'm going to be doing more scrubbing, writing tests, and measuring performance. In the meantime I welcome feedback.

Code that lived in WindowsFont moved mostly to TextExtensions and works directly on native GDI handles. The only known difference is that I currently don't cache the font height, but I'm planning to reintroduce that after I walk through the call chains.

cc: @Tanya-Solyanik, @KlausLoeffelmann, @RussKie

Microsoft Reviewers: Open in CodeFlow

@JeremyKuhne JeremyKuhne requested a review from a team as a code owner July 10, 2020 04:58
@ghost ghost assigned JeremyKuhne Jul 10, 2020
@JeremyKuhne JeremyKuhne marked this pull request as draft July 10, 2020 04:58
@JeremyKuhne JeremyKuhne reopened this Jul 10, 2020
@JeremyKuhne
Copy link
Member Author

I've found that we can save quite a bit of allocations and complexity by adding IDeviceContext to PaintEventArgs. Doing so allows us to smuggle the event through a lot of our APIs which actually only depend on (or are currently defined as) IDeviceContext. When we create the HDC scope for the device context we can check to see if we got the event and smartly avoid trying to apply clipping & transforms (which are costly).

The implementation would be simple and I wouldn't hang new public methods, but given that the interface is public this would still be an API change. Here is the implementation (which we never use directly, but would be rational if someone tried to use it):

        IntPtr IDeviceContext.GetHdc() => Graphics?.GetHdc() ?? IntPtr.Zero;
        void IDeviceContext.ReleaseHdc() => Graphics?.ReleaseHdc();

I think this is a reasonable thing to do given our dependence on IDeviceContext (which Graphics derives from, incidentally). It fits and if people want to leverage it to simplify their own logic it wouldn't be a bad thing (although they wouldn't get the full performance benefit we're getting, outside of not having to smuggle in a wrapper of some sort).

I'll have the PR updated with this later along with more numbers and other details.

@JeremyKuhne
Copy link
Member Author

On the same page as PaintEventArgs, we should also do the same optimizations on DrawItemEventArgs. We currently don't optimize for GDI drawing there and can do so by following the exact same pattern as PaintEventArgs does with this change.

The only semi-weird thing is that adding IDeviceContext brings IDisposable with it which would be a no-op. It would have to stay that way as it would be a pretty dramatic change to start taking ownership of the Graphics object. I'll live with it for the drop in allocations and we'd do an explicit implementation it shouldn't be rubbed in peoples faces.

@JeremyKuhne
Copy link
Member Author

I've updated the PR, but I need to merge conflicts again.

Here are current allocation numbers for invalidating and painting two different forms. The first "MulitpleControls" is in the controls test project.

Form .NET 4.8 Core 3.1 PreScopes Current
MultipleControls 24 KB 17.78 KB 17.77 KB 8.05 KB
ManyControls 32 KB 42.72 KB 38.89 KB 15.42 KB

"ManyControls" is one I've created with a number of controls as well. Here is what it looks like. I'm still iterating on what I've done and reviewing my change but the automated tests pass.

image

This change removes all of the misc\GDI classes and replaces them with
new isolated caches for Font-to-HFONT and "measurement" screen DCs.

This reduces significant allocations and complexity and improves performance
with these legacy code paths.

This also starts changing Graphics wrapping event args to derive from
IDeviceContext to allow unifying handling of extraction of HDCs via
DeviceContextHDCScope with low overhead.

Introduce internal IGraphicsHdcProvider to allow unwrapping
IDeviceContext derived paint events.

Convert a number of paths to use GDI where possible (notably when
there isn't transparency involved).

Optimize HBRUSH context to get system brushes when provided with a
color that is wrapping a system color.
@JeremyKuhne
Copy link
Member Author

The design for the changes in this PR is settled. I'm digging through the rest of the paint allocations and will be updating things like comments on classes/methods.

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

53% through...

More cleanup, breaking out classes, matching names, clarifying comments.

Add ARGB struct to deal with ARGB values and create Mix extension.

Add our own SolidBrush to avoid hooking events when created from System colors.
- Update IDeviceContext per API review approval
- Move HLSColor to separate file and cleanup
- Improve perf of ControlPaint.DrawBorder
- Misc cleanup
- Add DrawLines overloads
- Add StaticPen/Brush helpers to keep from hooking system colors
@JeremyKuhne JeremyKuhne marked this pull request as ready for review July 17, 2020 05:29
@JeremyKuhne
Copy link
Member Author

While I have plenty more to do after this PR, I think the PR here is ready for review.

Fundamental components:

  • Fully eliminates all existing finalizable GDI wrappers and caches (from GDI\misc)
  • Replaces DeviceContext and WindowsGraphics with DeviceContextHdcScope
  • Font to HFONT caching is contained in FontCache
  • Screen HDC's are now managed by ScreenDcCache
  • GdiCache is the entry point for GDI object caches
  • Starts implementing IDeviceContext on event args as a method for smuggling through existing IDeviceContext APIs to DeviceContextHdcScope, which looks for
    the internal IGraphicsHdcProvider that the event args also implement
  • HDC/Graphics optimization is wrapped in DrawingEventArgs struct that events can contain to manage consistently
  • Hacky extension TryGetGraphics() allows us to refactor APIs to use IDeviceContext when there is still a dependency on Graphics for some code paths (e.g. transparent color drawing)
  • Add a few new drawing helpers in DeviceContextExtensions
  • Some initial fiddling with GDI+ direct usage- will iterate on where that lands in upcoming PRs
  • Some APIs no longer throw ArgumentNullException, updated a number of tests
  • Various style cleanups as I was going through investigating call chains
  • Eliminate unnecessary state saving or Graphics wrapping on a number of call sites

@JeremyKuhne
Copy link
Member Author

@weltkante this change should mitigate the wacky HDC scoping issues we've been seeing in the tests- since you've been seeing this you might want to have a look at the caches I've introduced here to move everything to stack based scopes.

@weltkante
Copy link
Contributor

@JeremyKuhne thanks, will have a look and also try your PR later and see if the other problems get resolved

Copy link
Member

@RussKie RussKie left a comment

Choose a reason for hiding this comment

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

Massive work 🚀

@ghost ghost added the 📭 waiting-author-feedback The team requires more information from the author label Jul 17, 2020
@weltkante
Copy link
Contributor

weltkante commented Jul 17, 2020

The replacement code for #3391 and #3450 looks good (except the point noted above) and should fix the respective issues. Can't really test the way I did before since there is no GC involved anymore, so nothing to leak into a finalizer I can put a breakpoint on ;-) but manual inspection of the source looks good. Feel free to add both issues to the PR description for resolution if you want.

@ghost ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jul 17, 2020
Clean up ControlPaint
Copy link
Member

@KlausLoeffelmann KlausLoeffelmann left a comment

Choose a reason for hiding this comment

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

We did an 1:1 teams code review.
Looked very good to me!

Copy link
Member

@KlausLoeffelmann KlausLoeffelmann left a comment

Choose a reason for hiding this comment

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

And...checked the tests.

@JeremyKuhne JeremyKuhne merged commit a6a308f into dotnet:master Jul 18, 2020
@ghost ghost added this to the 5.0 RC1 milestone Jul 18, 2020
@JeremyKuhne
Copy link
Member Author

Fixes #3391, #3450

@hughbe
Copy link
Contributor

hughbe commented Jan 16, 2021

We should look into removing WindowsGraphics in System.Drawing.Common over in dotnet/runtime too at some point

@dotnet dotnet locked as resolved and limited conversation to collaborators Jan 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants