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 improvements to FrugalList #6280

Merged
merged 2 commits into from
Jul 21, 2022
Merged

Conversation

stephentoub
Copy link
Member

Description

  • FrugalStructList's ICollection<T>-based constructor uses foreach to enumerate the contents of the collection. If it's an IList<T>, we can instead index and avoid allocating the enumerator.
  • Avoid multiple interface calls to ICollection<T>.Count in FrugalStructList's ctor
  • Delete a dead ctor on ArrayItemList<T>. That ctor was the only reason an array field may have been left null, so we can also remove subsequent null checks when accessing that array.
  • Use Span/Array in ArrayItemList for Clear, Contains, IndexOf, ToArray, and CopyTo rather than open-coding them

Customer Impact

Unnecessary allocation and interface dispatch

Regression

No

Testing

CI

Risk

Minimal

- FrugalStructList's `ICollection<T>`-based constructor uses foreach to enumerate the contents of the collection.  If it's an `IList<T>`, we can instead index and avoid allocating the enumerator.
- Avoid multiple interface calls to `ICollection<T>.Count` in FrugalStructList's ctor
- Delete a dead ctor on `ArrayItemList<T>`.  That ctor was the only reason an array field may have been left null, so we can also remove subsequent null checks when accessing that array.
- Use Span/Array in ArrayItemList for Clear, Contains, IndexOf, ToArray, and CopyTo rather than open-coding them
@stephentoub stephentoub requested a review from a team as a code owner March 18, 2022 14:00
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Mar 18, 2022
@ghost ghost requested review from dipeshmsft, singhashish-wpf and SamBent March 18, 2022 14:00
bgrainger added a commit to Faithlife/wpf that referenced this pull request May 30, 2022
This commit was reverted in 3a54bfb.
@bgrainger
Copy link
Contributor

I haven't tracked down the issue yet, but when I cherry-picked this PR against release/6.0 (to produce a custom build of WPF for a .NET 6.0 application), it crashed at startup with the following exception:

System.ArgumentNullException: Value cannot be null. (Parameter 'genericHandler')
   at System.Windows.RoutedEventArgs.InvokeEventHandler(Delegate genericHandler, Object genericTarget) in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/RoutedEventArgs.cs:line 285
   at System.Windows.RoutedEventArgs.InvokeHandler(Delegate handler, Object target) in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/RoutedEventArgs.cs:line 336
   at System.Windows.EventRoute.InvokeHandlersImpl(Object source, RoutedEventArgs args, Boolean reRaised) in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/EventRoute.cs:line 207
   at System.Windows.EventRoute.InvokeHandlers(Object source, RoutedEventArgs args) in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/EventRoute.cs:line 128
   at System.Windows.UIElement.RaiseEventImpl(DependencyObject sender, RoutedEventArgs args) in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/UIElement.cs:line 2340
   at System.Windows.UIElement.RaiseEvent(RoutedEventArgs e) in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Generated/UIElement.cs:line 419
   at System.Windows.BroadcastEventHelper.BroadcastEvent(DependencyObject root, RoutedEvent routedEvent) in /_/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/BroadcastEventHelper.cs:line 280
   at System.Windows.BroadcastEventHelper.BroadcastLoadedSynchronously(DependencyObject rootDO, Boolean isLoaded) in /_/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/BroadcastEventHelper.cs:line 192
   at System.Windows.BroadcastEventHelper.BroadcastLoadedEvent(Object root) in /_/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/BroadcastEventHelper.cs:line 180
   at System.Windows.Media.MediaContext.FireLoadedPendingCallbacks() in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/MediaContext.cs:line 1993
   at System.Windows.Media.MediaContext.FireInvokeOnRenderCallbacks() in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/MediaContext.cs:line 1980
   at System.Windows.Media.MediaContext.RenderMessageHandlerCore(Object resizedCompositionTarget) in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/MediaContext.cs:line 1841
   at System.Windows.Media.MediaContext.RenderMessageHandler(Object resizedCompositionTarget) in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/MediaContext.cs:line 1744
   at System.Windows.Media.MediaContext.Resize(ICompositionTarget resizedCompositionTarget) in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/Media/MediaContext.cs:line 1727
   at System.Windows.Interop.HwndTarget.OnResize() in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/InterOp/HwndTarget.cs:line 1601
   at System.Windows.Interop.HwndTarget.HandleMessage(WindowMessage msg, IntPtr wparam, IntPtr lparam) in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/InterOp/HwndTarget.cs:line 1099
   at System.Windows.Interop.HwndSource.HwndTargetFilterMessage(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam, Boolean& handled) in /_/src/Microsoft.DotNet.Wpf/src/PresentationCore/System/Windows/InterOp/HwndSource.cs:line 1188
   at MS.Win32.HwndWrapper.WndProc(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam, Boolean& handled) in /_/src/Microsoft.DotNet.Wpf/src/Shared/MS/Win32/HwndWrapper.cs:line 295
   at MS.Win32.HwndSubclass.DispatcherCallbackOperation(Object o) in /_/src/Microsoft.DotNet.Wpf/src/Shared/MS/Win32/HwndSubclass.cs:line 429
   at System.Windows.Threading.ExceptionWrapper.InternalRealCall(Delegate callback, Object args, Int32 numArgs) in /_/src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/Threading/ExceptionWrapper.cs:line 103
   at System.Windows.Threading.ExceptionWrapper.TryCatchWhen(Object source, Delegate callback, Object args, Int32 numArgs, Delegate catchHandler) in /_/src/Microsoft.DotNet.Wpf/src/WindowsBase/MS/Internal/Threading/ExceptionWrapper.cs:line 36
   at System.Windows.Threading.Dispatcher.WrappedInvoke(Delegate callback, Object args, Int32 numArgs, Delegate catchHandler) in /_/src/Microsoft.DotNet.Wpf/src/WindowsBase/System/Windows/Threading/Dispatcher.cs:line 2859
   at System.Windows.Threading.Dispatcher.LegacyInvokeImpl(DispatcherPriority priority, TimeSpan timeout, Delegate method, Object args, Int32 numArgs) in /_/src/Microsoft.DotNet.Wpf/src/WindowsBase/System/Windows/Threading/Dispatcher.cs:line 1331
   at System.Windows.Threading.Dispatcher.Invoke(DispatcherPriority priority, Delegate method, Object arg) in /_/src/Microsoft.DotNet.Wpf/src/WindowsBase/System/Windows/Threading/Dispatcher.cs:line 1050
   at MS.Win32.HwndSubclass.SubclassWndProc(IntPtr hwnd, Int32 msg, IntPtr wParam, IntPtr lParam) in /_/src/Microsoft.DotNet.Wpf/src/Shared/MS/Win32/HwndSubclass.cs:line 341

Reverting this commit fixed the crash.

Co-authored-by: Bradley Grainger <bgrainger@gmail.com>
@ghost ghost assigned stephentoub May 30, 2022
@singhashish-wpf singhashish-wpf merged commit 9949509 into dotnet:main Jul 21, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants