Add PropertyItem tests #46794

hughbe · 2021-01-10T17:30:49Z

No description provided.

ghost · 2021-01-10T17:30:54Z

Tagging subscribers to this area: @safern, @tannergooding
See info in area-owners.md if you want to be subscribed.

Issue Details

Not committing the refactoring yet as I need to baseline the test failures on mac :)

Author:	hughbe
Assignees:	-
Labels:	`area-System.Drawing`
Milestone:	-

hughbe · 2021-01-11T13:45:53Z

Ok most of these tests fail on libgdiplus but will be fixed in mono/libgdiplus#683.

I'll disable them on mac soon

hughbe · 2021-01-12T14:59:16Z

Test failures unrelated - System.Speech problems

safern · 2021-01-13T21:36:15Z

@hughbe there are still a lot of System.Drawing test failures. https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-46794-merge-3d7963e8c70b4b2a86/System.Drawing.Common.Tests/console.2c2899a9.log?sv=2019-07-07&se=2021-02-01T11%3A35%3A29Z&sr=c&sp=rl&sig=tAUBVryl23%2BDhNnniL%2BghEMDUec5FpnVhdGh3nRBOWM%3D

hughbe · 2021-01-13T22:38:30Z

@safern yeah looks like we're running into #34592 annoyingly, so my changes won't work.

I've backed out the refactoring changes until that bug in the mono runtime is fixed!

hughbe · 2021-01-13T22:40:39Z

I may need to actually disable the tests - so tests will probably fail again then I will baseline the failures

safern · 2021-01-14T00:01:14Z

Sounds good. Let me know when/if this is ready for review 😄

hughbe · 2021-01-14T11:00:53Z

OK I've made the following changes that are actually quite positive.

Basically, I converted PropertyItemInternal into a blittable struct. Now, instead of marshalling lots of data we can just pin the PropertyItem.Value we simply pass the struct directly to the native code.

I also avoided an allocation of an array of one element in the code for GetPropertyItem.

I also took advantage of Span<byte>(ptr, len).ToArray() instead of doing Marshal.Copy which should also be better

This will have some perf wins and also shortens the code! Winning!

hughbe · 2021-01-14T13:01:28Z

Have run the tests on macOS with my locally built libgdiplus (enabling the property item tests that fail but are fixed in mono/libgdiplus#683).

So this should be now ready for review!

hugh@Hughs-MacBook-Air tests % /Users/hugh/Documents/GitHub/runtime/.dotnet/dotnet build /T:Test /P:XUnitOptions="-class System.Drawing.Imaging.Tests.PropertyItemTests"
Microsoft (R) Build Engine version 16.8.0+126527ff1 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
  System.Security.Principal.Windows -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.Principal.Windows/ref/netstandard2.0-Debug/System.Security.Principal.Windows.dll
  System.Security.AccessControl -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.AccessControl/ref/netstandard2.0-Debug/System.Security.AccessControl.dll
  Microsoft.Win32.Registry -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/Microsoft.Win32.Registry/ref/netstandard2.0-Debug/Microsoft.Win32.Registry.dll
  System.Runtime -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Runtime/ref/net6.0-Debug/System.Runtime.dll
  System.ComponentModel -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.ComponentModel/ref/net6.0-Debug/System.ComponentModel.dll
  Microsoft.Win32.Primitives -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/Microsoft.Win32.Primitives/ref/net6.0-Debug/Microsoft.Win32.Primitives.dll
  System.Security.Principal -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.Principal/ref/net6.0-Debug/System.Security.Principal.dll
  System.Collections -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Collections/ref/net6.0-Debug/System.Collections.dll
  System.Runtime.Extensions -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Runtime.Extensions/ref/net6.0-Debug/System.Runtime.Extensions.dll
  System.Runtime.InteropServices -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Runtime.InteropServices/ref/net6.0-Debug/System.Runtime.InteropServices.dll
  System.Collections.NonGeneric -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Collections.NonGeneric/ref/net6.0-Debug/System.Collections.NonGeneric.dll
  System.ObjectModel -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.ObjectModel/ref/net6.0-Debug/System.ObjectModel.dll
  System.Memory -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Memory/ref/net6.0-Debug/System.Memory.dll
  System.ComponentModel.Primitives -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.ComponentModel.Primitives/ref/net6.0-Debug/System.ComponentModel.Primitives.dll
  System.Net.Primitives -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Net.Primitives/ref/net6.0-Debug/System.Net.Primitives.dll
  System.Collections.Specialized -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Collections.Specialized/ref/net6.0-Debug/System.Collections.Specialized.dll
  System.Net.WebSockets -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Net.WebSockets/ref/net6.0-Debug/System.Net.WebSockets.dll
  System.Security.Claims -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.Claims/ref/net6.0-Debug/System.Security.Claims.dll
  System.Security.Principal.Windows -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.Principal.Windows/ref/net6.0-Debug/System.Security.Principal.Windows.dll
  System.Security.AccessControl -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.AccessControl/ref/net6.0-Debug/System.Security.AccessControl.dll
  TestUtilities -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/TestUtilities/net6.0-Debug/TestUtilities.dll
  Microsoft.Win32.SystemEvents -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/Microsoft.Win32.SystemEvents/netstandard2.0-Debug/Microsoft.Win32.SystemEvents.dll
  System.Drawing.Common -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Drawing.Common/net6.0-Unix-Debug/System.Drawing.Common.dll
  System.Drawing.Common.Tests -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Drawing.Common.Tests/net6.0-Unix-Debug/System.Drawing.Common.Tests.dll
  ----- start Thu Jan 14 12:58:29 GMT 2021 =============== To repro directly: =====================================================
  pushd /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Drawing.Common.Tests/net6.0-Unix-Debug
  /Users/hugh/Documents/GitHub/runtime/artifacts/bin/testhost/net6.0-OSX-Debug-x64/dotnet exec --runtimeconfig System.Drawing.Common.Tests.runtimeconfig.json --depsfile System.Drawing.Common.Tests.deps.json xunit.console.dll System.Drawing.Common.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing -class System.Drawing.Imaging.Tests.PropertyItemTests 
  popd
  ===========================================================================================================
  ~/Documents/GitHub/runtime/artifacts/bin/System.Drawing.Common.Tests/net6.0-Unix-Debug ~/Documents/GitHub/runtime/src/libraries/System.Drawing.Common/tests
    Discovering: System.Drawing.Common.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Drawing.Common.Tests (found 5 of 2033 test cases)
    Starting:    System.Drawing.Common.Tests (parallel test collections = on, max threads = 4)
    Finished:    System.Drawing.Common.Tests
  === TEST EXECUTION SUMMARY ===
     System.Drawing.Common.Tests  Total: 15, Errors: 0, Failed: 0, Skipped: 0, Time: 8.400s
  ~/Documents/GitHub/runtime/src/libraries/System.Drawing.Common/tests
  ----- end Thu Jan 14 12:59:23 GMT 2021 ----- exit code 0 ----------------------------------------------------------
  exit code 0 means Exited Successfully

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:01:19.55

hugh@Hughs-MacBook-Air tests % /Users/hugh/Documents/GitHub/runtime/.dotnet/dotnet build /T:Test /P:XUnitOptions="-class System.Drawing.Tests.ImageTests"
Microsoft (R) Build Engine version 16.8.0+126527ff1 for .NET
Copyright (C) Microsoft Corporation. All rights reserved.

  Determining projects to restore...
  All projects are up-to-date for restore.
  System.Security.Principal.Windows -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.Principal.Windows/ref/netstandard2.0-Debug/System.Security.Principal.Windows.dll
  System.Security.AccessControl -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.AccessControl/ref/netstandard2.0-Debug/System.Security.AccessControl.dll
  Microsoft.Win32.Registry -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/Microsoft.Win32.Registry/ref/netstandard2.0-Debug/Microsoft.Win32.Registry.dll
  System.Runtime -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Runtime/ref/net6.0-Debug/System.Runtime.dll
  System.ComponentModel -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.ComponentModel/ref/net6.0-Debug/System.ComponentModel.dll
  Microsoft.Win32.Primitives -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/Microsoft.Win32.Primitives/ref/net6.0-Debug/Microsoft.Win32.Primitives.dll
  System.Collections -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Collections/ref/net6.0-Debug/System.Collections.dll
  System.Runtime.Extensions -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Runtime.Extensions/ref/net6.0-Debug/System.Runtime.Extensions.dll
  System.Security.Principal -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.Principal/ref/net6.0-Debug/System.Security.Principal.dll
  System.ObjectModel -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.ObjectModel/ref/net6.0-Debug/System.ObjectModel.dll
  System.Collections.NonGeneric -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Collections.NonGeneric/ref/net6.0-Debug/System.Collections.NonGeneric.dll
  System.Runtime.InteropServices -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Runtime.InteropServices/ref/net6.0-Debug/System.Runtime.InteropServices.dll
  System.Memory -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Memory/ref/net6.0-Debug/System.Memory.dll
  System.ComponentModel.Primitives -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.ComponentModel.Primitives/ref/net6.0-Debug/System.ComponentModel.Primitives.dll
  System.Net.Primitives -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Net.Primitives/ref/net6.0-Debug/System.Net.Primitives.dll
  System.Collections.Specialized -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Collections.Specialized/ref/net6.0-Debug/System.Collections.Specialized.dll
  System.Net.WebSockets -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Net.WebSockets/ref/net6.0-Debug/System.Net.WebSockets.dll
  System.Security.Claims -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.Claims/ref/net6.0-Debug/System.Security.Claims.dll
  System.Security.Principal.Windows -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.Principal.Windows/ref/net6.0-Debug/System.Security.Principal.Windows.dll
  System.Security.AccessControl -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Security.AccessControl/ref/net6.0-Debug/System.Security.AccessControl.dll
  TestUtilities -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/TestUtilities/net6.0-Debug/TestUtilities.dll
  Microsoft.Win32.SystemEvents -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/Microsoft.Win32.SystemEvents/netstandard2.0-Debug/Microsoft.Win32.SystemEvents.dll
  System.Drawing.Common -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Drawing.Common/net6.0-Unix-Debug/System.Drawing.Common.dll
  System.Drawing.Common.Tests -> /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Drawing.Common.Tests/net6.0-Unix-Debug/System.Drawing.Common.Tests.dll
  ----- start Thu Jan 14 12:56:16 GMT 2021 =============== To repro directly: =====================================================
  pushd /Users/hugh/Documents/GitHub/runtime/artifacts/bin/System.Drawing.Common.Tests/net6.0-Unix-Debug
  /Users/hugh/Documents/GitHub/runtime/artifacts/bin/testhost/net6.0-OSX-Debug-x64/dotnet exec --runtimeconfig System.Drawing.Common.Tests.runtimeconfig.json --depsfile System.Drawing.Common.Tests.deps.json xunit.console.dll System.Drawing.Common.Tests.dll -xml testResults.xml -nologo -notrait category=OuterLoop -notrait category=failing -class System.Drawing.Tests.ImageTests 
  popd
  ===========================================================================================================
  ~/Documents/GitHub/runtime/artifacts/bin/System.Drawing.Common.Tests/net6.0-Unix-Debug ~/Documents/GitHub/runtime/src/libraries/System.Drawing.Common/tests
    Discovering: System.Drawing.Common.Tests (method display = ClassAndMethod, method display options = None)
    Discovered:  System.Drawing.Common.Tests (found 26 of 2033 test cases)
    Starting:    System.Drawing.Common.Tests (parallel test collections = on, max threads = 4)
    Finished:    System.Drawing.Common.Tests
  === TEST EXECUTION SUMMARY ===
     System.Drawing.Common.Tests  Total: 71, Errors: 0, Failed: 0, Skipped: 0, Time: 9.533s  ~/Documents/GitHub/runtime/src/libraries/System.Drawing.Common/tests
  ----- end Thu Jan 14 12:57:18 GMT 2021 ----- exit code 0 ----------------------------------------------------------
  exit code 0 means Exited Successfully

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:01:18.67

safern · 2021-01-29T21:38:59Z

src/libraries/System.Drawing.Common/src/System/Drawing/PropertyItemInternal.cs

-    // sdkinc\imaging.h
-    [StructLayout(LayoutKind.Sequential)]
-    internal sealed class PropertyItemInternal : IDisposable
+#pragma warning disable CS0649


Maybe just put it around the uninitialized field?

safern · 2021-01-29T21:41:37Z

src/libraries/System.Drawing.Common/tests/Imaging/PropertyItemTests.cs

+            item.Value = value;
+            Assert.Same(value, item.Value);
+
+            // Set same.


What is this exercising? I mean double setting to the same value?

Its something we do in winforms where we test setting a property to the same value to make sure things are idempotent.

Removed

hughbe · 2021-02-12T10:43:26Z

@JeremyKuhne also may be worth taking a look as you've done something similar here recently

JeremyKuhne · 2021-02-24T03:07:59Z

src/libraries/System.Drawing.Common/src/System/Drawing/Image.cs

+                if (size == 0 || count == 0)
+                    return Array.Empty<PropertyItem>();
+
+                PropertyItemInternal* propdata = (PropertyItemInternal*)Marshal.AllocHGlobal((int)size);


Can you change this to rent a byte array from the ArrayPool and pin that for the buffer please? It would be good to get rid of the AllocHGlobal. :)

JeremyKuhne · 2021-02-24T03:19:24Z

src/libraries/System.Drawing.Common/src/System/Drawing/Image.cs

+            if (size == 0)
+                return null;
+
+            PropertyItemInternal* propdata = (PropertyItemInternal*)Marshal.AllocHGlobal((int)size);


Rent a byte buffer from the ArrayPool here too.

safern

Thanks!

safern · 2021-03-26T01:05:44Z

I'll close and re-open as the build is 3 weeks old already.

safern · 2021-03-26T01:06:12Z

I hit the wrong button 😄 (twice). I'll keep an eye on the build

Dotnet-GitSync-Bot added the area-System.Drawing label Jan 10, 2021

hughbe force-pushed the propertyiteminternal branch 2 times, most recently from 80b7e2b to ec4a1b5 Compare January 12, 2021 10:36

hughbe force-pushed the propertyiteminternal branch from ec4a1b5 to 905a9ed Compare January 13, 2021 22:37

hughbe changed the title ~~Refactor PropertyItem image code~~ Add PropertyItem tests Jan 13, 2021

hughbe force-pushed the propertyiteminternal branch from 905a9ed to 440b283 Compare January 13, 2021 22:39

hughbe force-pushed the propertyiteminternal branch from 1ed8bb1 to 3102772 Compare January 14, 2021 10:59

hughbe force-pushed the propertyiteminternal branch from 3102772 to 77e3558 Compare January 14, 2021 12:43

runfoapp bot mentioned this pull request Jan 15, 2021

SslStreamNetworkStreamTest failing intermitently on OSX #46837

Closed

hughbe force-pushed the propertyiteminternal branch from 77e3558 to a51f503 Compare January 16, 2021 13:03

safern reviewed Jan 29, 2021

View reviewed changes

hughbe force-pushed the propertyiteminternal branch from a51f503 to 03bf0fc Compare February 12, 2021 10:41

hughbe force-pushed the propertyiteminternal branch from 03bf0fc to 8e40c51 Compare February 23, 2021 11:35

JeremyKuhne reviewed Feb 24, 2021

View reviewed changes

Base automatically changed from master to main March 1, 2021 09:07

hughbe added 2 commits March 7, 2021 13:22

Refactor PropertyItem image code

7422e05

Refactor PropertyItemInternal

1045095

hughbe added 3 commits March 7, 2021 13:22

Use Windows implementation of PropertyItem in Unix

0e5fcd9

PR feedback

a8ce3c6

Use ArrayPool

b3b0287

hughbe force-pushed the propertyiteminternal branch from 8e40c51 to b3b0287 Compare March 7, 2021 13:22

safern approved these changes Mar 26, 2021

View reviewed changes

safern merged commit ec2fedc into dotnet:main Mar 26, 2021

hughbe deleted the propertyiteminternal branch March 26, 2021 08:51

ghost locked as resolved and limited conversation to collaborators Apr 25, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PropertyItem tests #46794

Add PropertyItem tests #46794

hughbe commented Jan 10, 2021 •

edited

Loading

ghost commented Jan 10, 2021

hughbe commented Jan 11, 2021

hughbe commented Jan 12, 2021

safern commented Jan 13, 2021

hughbe commented Jan 13, 2021

hughbe commented Jan 13, 2021

safern commented Jan 14, 2021

hughbe commented Jan 14, 2021

hughbe commented Jan 14, 2021

safern Jan 29, 2021

hughbe Feb 12, 2021

safern Jan 29, 2021

hughbe Feb 12, 2021

hughbe commented Feb 12, 2021

JeremyKuhne Feb 24, 2021

JeremyKuhne Feb 24, 2021

safern left a comment

safern commented Mar 26, 2021

safern commented Mar 26, 2021

Add PropertyItem tests #46794

Add PropertyItem tests #46794

Conversation

hughbe commented Jan 10, 2021 • edited Loading

ghost commented Jan 10, 2021

hughbe commented Jan 11, 2021

hughbe commented Jan 12, 2021

safern commented Jan 13, 2021

hughbe commented Jan 13, 2021

hughbe commented Jan 13, 2021

safern commented Jan 14, 2021

hughbe commented Jan 14, 2021

hughbe commented Jan 14, 2021

safern Jan 29, 2021

Choose a reason for hiding this comment

hughbe Feb 12, 2021

Choose a reason for hiding this comment

safern Jan 29, 2021

Choose a reason for hiding this comment

hughbe Feb 12, 2021

Choose a reason for hiding this comment

hughbe commented Feb 12, 2021

JeremyKuhne Feb 24, 2021

Choose a reason for hiding this comment

JeremyKuhne Feb 24, 2021

Choose a reason for hiding this comment

safern left a comment

Choose a reason for hiding this comment

safern commented Mar 26, 2021

safern commented Mar 26, 2021

hughbe commented Jan 10, 2021 •

edited

Loading