Feature/net 6 support #62

kenny-sellers · 2022-01-07T19:53:45Z

Fixes: #46

Description

Adding .NET 6 Support

Merge Checklist

Added unit or integration tests (if not explain)
Benchmarks are equivalent or faster

SkyeHoefling

Thanks for the contribution, this looks good overall. I have a couple notes that I would like resolved.

In the meantime, I am going to see what I can do about the benchmarking to see if we can include it in this PR

SkyeHoefling · 2022-01-07T20:05:42Z

.github/workflows/build_main.yml

+            framework: 'net6'
+          - os: windows-2019
+            architecture: 'x64'
+            framework: 'net6'


The winforms build is out of sync with what is happening in the PR build. Can you take a look and make sure the build_main.yml is correct. There should only be 1 build command like this:

- name: Build WinForms App run: dotnet build -c Release /p:Platform=${{ matrix.architecture }} working-directory: ./samples/WinformsApp/WinformsApp.${{ matrix.framework }}

SkyeHoefling · 2022-01-07T20:07:23Z

FileOnQ.Imaging.Heif.sln

-# Visual Studio Version 16
-VisualStudioVersion = 16.0.30114.105
+# Visual Studio Version 17
+VisualStudioVersion = 17.0.32014.148


This project doesn't official support Visual Studio 2022, this statement should not be updated. The C++ code fails when using the compilers in Visual Studio 2022 and we only support 2019.

…ild.

…ment

…aging.Heif into feature/net-6-support

SkyeHoefling · 2022-01-10T14:57:11Z

/benchmark

SkyeHoefling · 2022-01-10T15:01:15Z

I updated the benchmark project to support running under net48, net5, and net6. After a successful run we will need to update the saved results as the table will be slightly different from what is stored in the repository. The benefit here is we will be able to look at differences between all our supported platforms.

Let's wait and see with the CI says, I noticed locally there is a very small memory leak in net6 and net48 - under 200 bytes

SkyeHoefling · 2022-01-10T15:34:57Z

benchmarks failed because .NET 6 wasn't installed on job benchmark-run. I fixed this in main, let's try running them again.

SkyeHoefling · 2022-01-10T15:35:02Z

/benchmark

SkyeHoefling · 2022-01-10T16:37:53Z

I just peaked at the result of the benchmark build for Thumbnails and it includes net48 and net6 but not net 6. We need to add another ensure .NET SDK command to ensure .NET 5 is installed. See output https://github.com/FileOnQ/Imaging.Heif/actions/runs/1678150679

Method	Job	Runtime	Mean	Error	StdDev	Median	Gen 0	Gen 1	Gen 2	Allocated	Allocated native memory	Native memory leak
Thumbnail_Write	Job-RFREVW	.NET 5.0	NA	NA	NA	NA	-	-	-	-	-	-
Thumbnail_ToArray	Job-RFREVW	.NET 5.0	NA	NA	NA	NA	-	-	-	-	-	-
Thumbnail_ToSpan	Job-RFREVW	.NET 5.0	NA	NA	NA	NA	-	-	-	-	-	-
Thumbnail_ToStream	Job-RFREVW	.NET 5.0	NA	NA	NA	NA	-	-	-	-	-	-
Thumbnail_Write	Job-LTLQYG	.NET 6.0	55.58 ms	1.052 ms	1.475 ms	55.03 ms	-	-	-	832 B	5,125,137 B	-
Thumbnail_ToArray	Job-LTLQYG	.NET 6.0	55.67 ms	1.075 ms	1.103 ms	55.67 ms	-	-	-	66,888 B	5,124,325 B	-
Thumbnail_ToSpan	Job-LTLQYG	.NET 6.0	53.05 ms	1.054 ms	1.732 ms	52.23 ms	-	-	-	600 B	5,124,581 B	-
Thumbnail_ToStream	Job-LTLQYG	.NET 6.0	52.51 ms	0.986 ms	0.922 ms	52.56 ms	-	-	-	66,952 B	5,124,581 B	-
Thumbnail_Write	Job-UALGFG	.NET Framework 4.8	53.53 ms	1.062 ms	2.376 ms	52.91 ms	-	-	-	74,504 B	5,125,459 B	292 B
Thumbnail_ToArray	Job-UALGFG	.NET Framework 4.8	54.69 ms	1.079 ms	2.627 ms	53.92 ms	-	-	-	74,504 B	5,125,163 B	292 B
Thumbnail_ToSpan	Job-UALGFG	.NET Framework 4.8	53.01 ms	1.035 ms	1.382 ms	52.94 ms	-	-	-	8,192 B	5,125,163 B	292 B
Thumbnail_ToStream	Job-UALGFG	.NET Framework 4.8	55.46 ms	1.107 ms	2.133 ms	55.69 ms	-	-	-	140,816 B	5,125,163 B	292 B

SkyeHoefling · 2022-01-10T16:40:12Z

/benchmark

SkyeHoefling · 2022-01-10T21:53:27Z

@kenny-sellers can you take a look at the memory leak noted for .NET 4.8

kenny-sellers · 2022-01-10T22:08:36Z

@ahoefling Just for documentation purposes. We are also seeing a memory leak with Primary images. That one is 234B.

SkyeHoefling · 2022-01-11T13:53:41Z

Thanks for confirming the issue. I don't think we should have the memory leak or benchmarking hold up this PR. @kenny-sellers can you create a new issue to track the memory leak and include the .NET Framework benchmarks to document the problem.

I will make another pass at the code here and see what we need to do to close this out

SkyeHoefling

@kenny-sellers I noted all the benchmark changes that need to be reverted, can you make the changes and update the PR

SkyeHoefling · 2022-01-11T13:54:15Z

.github/workflows/benchmark.yml

@@ -3,7 +3,7 @@ name: Benchmark
 on:
  pull_request_target:
    branches: [ main ]
-    types: [ opened ]
+    types: [ opened, synchronize ]


Remove testing action trigger

Suggested change

types: [ opened, synchronize ]

types: [ opened ]

SkyeHoefling · 2022-01-11T13:55:01Z

.github/workflows/benchmark.yml

@@ -73,7 +73,7 @@ jobs:
      - name: Setup .NET SDK
        uses: actions/setup-dotnet@v1
        with:
-          dotnet-version: 5.0.302
+          dotnet-version: 6.0.101


Revert benchmark build changes, this will be captured in another work item

Suggested change

dotnet-version: 6.0.101

dotnet-version: 5.0.302

SkyeHoefling · 2022-01-11T13:55:33Z

.github/workflows/benchmark.yml

+            architecture: 'AnyCPU'
+            platform: 'net5'


Revert benchmark build changes, this will be captured in another work item

Suggested change

architecture: 'AnyCPU'

platform: 'net5'

SkyeHoefling · 2022-01-11T13:55:56Z

.github/workflows/benchmark.yml

@@ -135,7 +137,7 @@ jobs:
        working-directory: ./benchmarks/tools

      - name: Benchmark
-        run: dotnet run -c Release -b ${{ matrix.benchmark }}
+        run: dotnet run -c Release -f net6.0 -b ${{ matrix.benchmark }}


Revert benchmark build changes, this will be captured in another work item

Suggested change

run: dotnet run -c Release -f net6.0 -b ${{ matrix.benchmark }}

run: dotnet run -c Release -b ${{ matrix.benchmark }}

SkyeHoefling · 2022-01-11T13:57:33Z

benchmarks/tools/Build/nuget-local.targets

@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <Project ToolsVersion="15.0">

-	<Target Name="ClearPackageCache" BeforeTargets="Build" Condition="'$(CI)' == 'false'">
+	<Target Name="ClearPackageCache" BeforeTargets="DispatchToInnerBuilds" Condition="'$(CI)' == 'false'">


Revert benchmark changes

Suggested change

<Target Name="ClearPackageCache" BeforeTargets="DispatchToInnerBuilds" Condition="'$(CI)' == 'false'">

<Target Name="ClearPackageCache" BeforeTargets="Build" Condition="'$(CI)' == 'false'">

SkyeHoefling · 2022-01-11T13:57:54Z

benchmarks/tools/FileOnQ.Imaging.Heif.Benchmarks.csproj

@@ -2,7 +2,7 @@

  <PropertyGroup>
    <OutputType>Exe</OutputType>
-    <TargetFramework>net5.0</TargetFramework>
+    <TargetFrameworks>net48;net5.0;net6.0</TargetFrameworks>


Revert benchmark changes

Suggested change

<TargetFrameworks>net48;net5.0;net6.0</TargetFrameworks>

<TargetFramework>net5.0</TargetFramework>

SkyeHoefling · 2022-01-11T13:58:11Z

benchmarks/tools/PrimaryImage.cs

@@ -7,7 +7,9 @@

 namespace FileOnQ.Imaging.Heif.Benchmarks
 {
+	[SimpleJob(RuntimeMoniker.Net48, launchCount: 1, invocationCount: 1)]


Revert benchmark changes

Suggested change

[SimpleJob(RuntimeMoniker.Net48, launchCount: 1, invocationCount: 1)]

SkyeHoefling · 2022-01-11T13:58:15Z

benchmarks/tools/PrimaryImage.cs

 	[SimpleJob(RuntimeMoniker.Net50, launchCount: 1, invocationCount: 1)]
+	[SimpleJob(RuntimeMoniker.Net60, launchCount: 1, invocationCount: 1)]


Revert benchmark changes

Suggested change

[SimpleJob(RuntimeMoniker.Net60, launchCount: 1, invocationCount: 1)]

SkyeHoefling · 2022-01-11T13:58:20Z

benchmarks/tools/Thumbnail.cs

@@ -7,7 +7,9 @@

 namespace FileOnQ.Imaging.Heif.Benchmarks
 {
+	[SimpleJob(RuntimeMoniker.Net48, launchCount: 1, invocationCount: 1)]


Revert benchmark changes

Suggested change

[SimpleJob(RuntimeMoniker.Net48, launchCount: 1, invocationCount: 1)]

SkyeHoefling · 2022-01-11T13:58:24Z

benchmarks/tools/Thumbnail.cs

 	[SimpleJob(RuntimeMoniker.Net50, launchCount: 1, invocationCount: 1)]
+	[SimpleJob(RuntimeMoniker.Net60, launchCount: 1, invocationCount: 1)]


Revert benchmark changes

Suggested change

[SimpleJob(RuntimeMoniker.Net60, launchCount: 1, invocationCount: 1)]

@ahoefling I pushed the reverts!

SkyeHoefling · 2022-01-11T14:45:13Z

/benchmark

SkyeHoefling · 2022-01-11T14:45:33Z

As long as the builds and benchmarks succeed, we can merge this PR

SkyeHoefling · 2022-01-11T14:59:32Z

Benchmark is failing, because we need the script updated in main. We will clean this up in #66. If the current build passes we will merge this PR

kenny-sellers added 6 commits January 7, 2022 09:44

Adding .net 6 support for all projects.

fbe218d

Adding .net6 winforms app

1312dce

Add build steps for the .net 6 targeting

9074a84

removing rogue comma

11b840d

Other rogue comma

d0095f8

Correcting .NET SDK Version in actions.

b1fb624

SkyeHoefling suggested changes Jan 7, 2022

View reviewed changes

kenny-sellers added 2 commits January 7, 2022 14:14

Correcting Build to reflect the correct structure for winforms app bu…

f888afb

…ild.

Correcting Visual Studio Version to be VS 2019 due to the C++ require…

32c9251

…ment

kenny-sellers requested a review from SkyeHoefling January 7, 2022 20:16

SkyeHoefling added 5 commits January 10, 2022 09:51

Updated benchmark project to run benchmarks on net48, net5, and net6

432d2c6

Global.json set to use lowest net6 cli

8f29ad5

Updated build to specify net6 for benchmark runtime

c1a9c1a

Set synchronize action for benchmark build so we can test in PR

dfaaf93

Merge branch 'feature/net-6-support' of https://github.com/FileOnQ/Im…

e5ff9b3

…aging.Heif into feature/net-6-support

Adding .net6 SDK install step to PR and main build'

a5c8028

SkyeHoefling suggested changes Jan 11, 2022

View reviewed changes

Reverting benchmark changes. Changes will be made in a later PR

bdf73c9

SkyeHoefling mentioned this pull request Jan 11, 2022

.NET 4.8 Memory Leak #65

Closed

SkyeHoefling and others added 2 commits January 11, 2022 09:37

Merge branch 'main' into feature/net-6-support

6655bf0

Added .NET 6 sdk for benchmark build

1465a28

Cleaned up last review items for .NET 6

0fa5745

SkyeHoefling mentioned this pull request Jan 11, 2022

Add Benchmarks for .NET 6 and .NET Framework 4.8 #66

Closed

SkyeHoefling merged commit 61e0e6d into main Jan 11, 2022

SkyeHoefling deleted the feature/net-6-support branch January 11, 2022 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/net 6 support #62

Feature/net 6 support #62

kenny-sellers commented Jan 7, 2022 •

edited by SkyeHoefling

Loading

SkyeHoefling left a comment

SkyeHoefling Jan 7, 2022

SkyeHoefling Jan 7, 2022

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

kenny-sellers commented Jan 10, 2022

SkyeHoefling commented Jan 11, 2022

SkyeHoefling left a comment

SkyeHoefling Jan 11, 2022

SkyeHoefling Jan 11, 2022

SkyeHoefling Jan 11, 2022

SkyeHoefling Jan 11, 2022

SkyeHoefling Jan 11, 2022

SkyeHoefling Jan 11, 2022

SkyeHoefling Jan 11, 2022

SkyeHoefling Jan 11, 2022

SkyeHoefling Jan 11, 2022

SkyeHoefling Jan 11, 2022

kenny-sellers Jan 11, 2022

SkyeHoefling commented Jan 11, 2022

SkyeHoefling commented Jan 11, 2022

SkyeHoefling commented Jan 11, 2022

	run: dotnet run -c Release -f net6.0 -b ${{ matrix.benchmark }}
	run: dotnet run -c Release -b ${{ matrix.benchmark }}

	<Target Name="ClearPackageCache" BeforeTargets="DispatchToInnerBuilds" Condition="'$(CI)' == 'false'">
	<Target Name="ClearPackageCache" BeforeTargets="Build" Condition="'$(CI)' == 'false'">

	<TargetFrameworks>net48;net5.0;net6.0</TargetFrameworks>
	<TargetFramework>net5.0</TargetFramework>

		[SimpleJob(RuntimeMoniker.Net50, launchCount: 1, invocationCount: 1)]
		[SimpleJob(RuntimeMoniker.Net60, launchCount: 1, invocationCount: 1)]

Feature/net 6 support #62

Feature/net 6 support #62

Conversation

kenny-sellers commented Jan 7, 2022 • edited by SkyeHoefling Loading

Description

Merge Checklist

SkyeHoefling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

SkyeHoefling commented Jan 10, 2022

kenny-sellers commented Jan 10, 2022

SkyeHoefling commented Jan 11, 2022

SkyeHoefling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SkyeHoefling commented Jan 11, 2022

SkyeHoefling commented Jan 11, 2022

SkyeHoefling commented Jan 11, 2022

kenny-sellers commented Jan 7, 2022 •

edited by SkyeHoefling

Loading