[Windows] Optimize WrapperView#24281
Conversation
|
Hey there @MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
| DisposeShadow(); | ||
| DisposeBorder(); | ||
|
|
||
| GC.SuppressFinalize(this); |
There was a problem hiding this comment.
@jonathanpeppers is this the correct thing to do? Do we trust the AI?
There was a problem hiding this comment.
It's suggested by a dotnet analyzer FTR
There was a problem hiding this comment.
-
I think to finish out this pattern we need to add a finalizer as well https://learn.microsoft.com/en-us/dotnet/api/system.idisposable?view=net-8.0#examples
-
I'm not really a fan of this implementing IDisposable at all, but I don't think we can really do anything about that now
There was a problem hiding this comment.
I removed that change in be1ecb2 to avoid increasing scope of this PR.
| if (_borderPath is null) | ||
| { | ||
| _borderPath = new(); | ||
| Children.Add(_borderPath); | ||
| } |
There was a problem hiding this comment.
If not initialized, initialize now. A part of the main change.
| if (_shadowCanvas is null) | ||
| { | ||
| _shadowCanvas = new(); | ||
| Children.Add(_shadowCanvas); | ||
| } |
There was a problem hiding this comment.
A part of the main change.
| { | ||
| public partial class WrapperView : Grid, IDisposable | ||
| { | ||
| readonly Canvas _shadowCanvas; |
There was a problem hiding this comment.
Note: It was readonly but then _shadowCanvas was checked if it is null or not.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
A bunch of tests failed, I retried them now but if they still fail that might be something to look into |
Yes, the PR changes the behavior, it can be seen with
Working on fixing that. |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Azure Pipelines successfully started running 3 pipeline(s). |
samhouts
added
fixed-in-net9.0-nightly
MartyIX
deleted the
feature/2024-08-16-Windows-WrapperView-optimize-FINAL
branch
6 participants
Description of Change
This PR attempts to optimize
WrapperViewclass on Windows. Not everyWrapperViewhas a border or a shadow set, so it can be set up when it's really needed.Performance impact
-> 93% improvement for the execution of the constructor.
Issues Fixed
WrapperView#24281Contributes to [WinUI] Creating a complex grid is very slow #21787 (when
<Label>s have set a background color and thus there is a container for those labels).