Upgrade .NET version and harmonize versions of referenced packages #237

devtekve · 2024-05-10T10:26:42Z

This commit makes several changes to the .NET projects and their dependencies. The main changes include upgrading the projects to target newer versions of .NET (.NET 6.0, 7.0 and 8.0), and harmonize versions of referenced packages.

The package versions are now based on the target framework. The git diff shows the removal and addition of PackageReference tags that ensure packages have different versions based on the targeted framework. For example, Microsoft.NET.Test.Sdk is upgraded from 16.9.4 to 17.9.0 if the target framework is not net6.0.

In addition, the commit modifies the projects to produce an executable as the output type instead of a class library and removes constructors in exception classes which are obsolete from .net8 onwards (dotnet/docs#34893).

Finally, some refactoring is done to comply with the latest .net requirements regarding value and ref types.

Overall, this commit ensures the projects align with the latest .NET platform, benefiting from performance improvements and other advancements, while keeping the project dependencies clean and consistent.

Benchmark

.net7.0

BenchmarkDotNet v0.13.12, macOS Sonoma 14.3.1 (23D60) [Darwin 23.3.0]
Apple M1 Pro, 1 CPU, 10 logical and 10 physical cores
.NET SDK 8.0.201
  [Host]     : .NET 7.0.16 (7.0.1624.6629), Arm64 RyuJIT AdvSIMD
  DefaultJob : .NET 7.0.16 (7.0.1624.6629), Arm64 RyuJIT AdvSIMD


| Method            | EntitiesCount | Mean         | Error      | StdDev     | Gen0      | Gen1      | Gen2     | Allocated   |
|------------------ |-------------- |-------------:|-----------:|-----------:|----------:|----------:|---------:|------------:|
| EfficientDynamoDb | 10            |     33.07 us |   0.384 us |   0.359 us |    2.8076 |         - |        - |    17.23 KB |
| aws-sdk-net       | 10            |    309.88 us |   2.384 us |   2.113 us |   51.2695 |   13.1836 |        - |   315.54 KB |
| EfficientDynamoDb | 100           |    247.07 us |   3.939 us |   3.684 us |   19.5313 |    3.9063 |        - |   119.89 KB |
| aws-sdk-net       | 100           |  3,334.25 us |  47.241 us |  41.878 us |  480.4688 |  210.9375 |        - |  2958.24 KB |
| EfficientDynamoDb | 1000          |  2,468.43 us |  16.851 us |  15.762 us |  183.5938 |   89.8438 |        - |  1146.49 KB |
| aws-sdk-net       | 1000          | 48,140.56 us | 700.329 us | 620.823 us | 5090.9091 | 1363.6364 | 454.5455 | 29379.18 KB |

.net8.0

BenchmarkDotNet v0.13.12, macOS Sonoma 14.3.1 (23D60) [Darwin 23.3.0]
Apple M1 Pro, 1 CPU, 10 logical and 10 physical cores
.NET SDK 8.0.201
  [Host]     : .NET 8.0.2 (8.0.224.6711), Arm64 RyuJIT AdvSIMD
  DefaultJob : .NET 8.0.2 (8.0.224.6711), Arm64 RyuJIT AdvSIMD


| Method            | EntitiesCount | Mean         | Error      | StdDev     | Gen0      | Gen1      | Gen2      | Allocated   |
|------------------ |-------------- |-------------:|-----------:|-----------:|----------:|----------:|----------:|------------:|
| EfficientDynamoDb | 10            |     25.87 us |   0.252 us |   0.211 us |    2.7771 |    0.0916 |         - |    17.17 KB |
| aws-sdk-net       | 10            |    207.30 us |   3.084 us |   2.885 us |   51.2695 |   13.1836 |         - |   314.92 KB |
| EfficientDynamoDb | 100           |    187.08 us |   1.162 us |   1.030 us |   19.5313 |    0.2441 |         - |   119.84 KB |
| aws-sdk-net       | 100           |  2,459.85 us |  48.444 us | 119.743 us |  476.5625 |  234.3750 |         - |  2956.09 KB |
| EfficientDynamoDb | 1000          |  2,107.96 us |  41.388 us |  66.833 us |  183.5938 |   89.8438 |         - |  1146.55 KB |
| aws-sdk-net       | 1000          | 40,097.83 us | 494.365 us | 412.817 us | 4833.3333 | 1250.0000 | 1083.3333 | 29341.09 KB |

Note:

I don't have available a proper x86 setup where I could run the benchmark for the older dotnet versions, so I can't provide a proper comparison between current .netstandard2.1 and latest .net8. The improvement is still in line with the comparison against aws sdk, which also sees improvements by bumping the framework. So I see an overall performance gain.

If anyone is able to, can you please run the benchmarks on your system to see how it compares against the current version used? thank you :)

This commit makes several changes to the .NET projects and their dependencies. The main changes include upgrading the projects to target newer versions of .NET (.NET 6.0, 7.0 and 8.0), and harmonize versions of referenced packages. The package versions are now based on the target framework. The git diff shows the removal and addition of PackageReference tags that ensure packages have different versions based on the targeted framework. For example, `Microsoft.NET.Test.Sdk` is upgraded from 16.9.4 to 17.9.0 if the target framework is not net6.0. In addition, the commit modifies the projects to produce an executable as the output type instead of a class library and removes constructors in exception classes which are obsolete from .net8 onwards (dotnet/docs#34893). Finally, some refactoring is done to comply with the latest .net requirements regarding value and ref types. Overall, this commit ensures the projects align with the latest .NET platform, benefiting from performance improvements and other advancements, while keeping the project dependencies clean and consistent.

firenero · 2024-05-11T01:00:29Z

Thanks for submitting the PR @devtekve. Changing target version is a breaking change. I'm ok with doing that but we'll need to think about all breaking changes we want to make and bundle them into a single release. And before doing an upgrade I would like to release already implemented features to make sure we don't block anyone waiting for them.

And couple of notes about the update itself:

I don't want to support multiple targets and deal with compiler directives. In my experience they are usually a pain to support and I'd rather pick one target.
.NET 7 goes out of support in several days on May 14th. .NET 6's end of support is in November 2024, so it only has 6 months of lifetime. Taking these timelines into account I think it make sense to only support .NET 8 going forward and drop all previous versions.
Performance benefits you've observed in your benchmarks are achievable without updating the library's target framework. If your code runs on .NET 8 you'll get the runtime benefits regardless the library version. You can test it using the benchmarkdotnet. To do it, add the [SimpleJob(RuntimeMoniker.Net80)] and [SimpleJob(RuntimeMoniker.Net70)] (or more other) attributes to the class that contains benchmarks without changing the target version of EfficientDynamoDb. You may need to update the BenchmarkDotNet version to get enum values for latest runtimes.
An actual performance benefit from changing the target framework can come from:
1. Using newer more performant APIs from latest .NET versions
2. If any of the libraries we use in our code uses the newer faster API that's only available in latest .NET versions. This is most likely the case with System.Text.Json as it was improved quite a lot recently.

Overall, I think we need to bump the version to .net 8 but I'm not sure that right now is the best moment to do it. If you're up to it, let's leave your PR open and update it to only target .NET 8. And we can merge it after doing all necessary preparations.

devtekve · 2024-05-11T02:09:24Z

Definitely up for it. Only reason I added the multiple frameworks was to keep it somewhat backwards compatible. But I'd simply target .net8.0 as it's the latest LTS.

Also I am agree it's a breaking change, but in the other hand, the other runtimes are no longer supported. (Well, .netstandard2.1 is, but I found one bug regarding passing value types as references... latest .net8 is against it and yet .netstandard2.1 seems happy with it )

We can leave it open and polish it, get it ready and properly flagged as breaking change... to be honest, none of the PRs I made had the expectation of being merged as is, I expected some discussion since I am brand new to the project and still getting the hang of it but I'm really loving what you've done here hahahah

This commit removes the targeting of .NET 7.0 and 6.0 in all projects, and updates them to target .NET 8.0 only. It also updates the versions of package references according to this change.

…ss (we should maybe format it once, as it is inconsistent, but doesn't really matter much)

…work check

devtekve · 2024-05-11T13:54:22Z

src/EfficientDynamoDb.Credentials.AWSSDK/EfficientDynamoDb.Credentials.AWSSDK.csproj

-      <PackageReference Include="EfficientDynamoDb" Version="0.9.2-alpha.0.4" />
-      <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.1.1">
+      <PackageReference Include="AWSSDK.Core" Version="3.7.303.28" />
+      <PackageReference Include="EfficientDynamoDb" Version="0.9.15" />


Should we reference the package from local instead of a versioned package here? or doesn't matter?

EfficientDynamoDb.Credentials.AWSSDK should be updated separately after the main project is released. It will require a newly released version of EfficientDynamoDb because it's based on dotnet 8. Since it's coming from nuget we need to release the main library first.

So I'd suggest reverting changes to this project and bringing them back in another PR after the release.

devtekve · 2024-05-11T13:56:48Z

@firenero I've updated the PR removing many whitespaces changes on the csprojs, and removed the multiple .net frameworks from the targets, leaving only .net8

firenero · 2024-05-25T17:37:09Z

src/EfficientDynamoDb.Credentials.AWSSDK/EfficientDynamoDb.Credentials.AWSSDK.csproj

-      <PackageReference Include="EfficientDynamoDb" Version="0.9.2-alpha.0.4" />
-      <PackageReference Include="Microsoft.SourceLink.GitHub" Version="1.1.1">
+      <PackageReference Include="AWSSDK.Core" Version="3.7.303.28" />
+      <PackageReference Include="EfficientDynamoDb" Version="0.9.15" />


EfficientDynamoDb.Credentials.AWSSDK should be updated separately after the main project is released. It will require a newly released version of EfficientDynamoDb because it's based on dotnet 8. Since it's coming from nuget we need to release the main library first.

So I'd suggest reverting changes to this project and bringing them back in another PR after the release.

firenero · 2024-05-25T18:30:33Z

src/EfficientDynamoDb/Internal/Converters/PaginationDdbConverter.cs

+        private JsonReaderState _initialReaderState;
+        private DdbEntityReadStack _state;


Converters are supposed to be thread-safe because they are cached across multiple calls. This state will be shared with multiple callers that makes it impossible to use the converter from multiple threads.

firenero · 2024-05-25T18:34:26Z

src/EfficientDynamoDb/Internal/Converters/PaginationDdbConverter.cs

-
+                    _state = reader.State;
+                    var originalSpan = new ReadOnlySpan<byte>(_state.Buffer, _state.BufferStart, _state.BufferLength);
+                    reader = new DdbReader(originalSpan[offset..], reader.JsonReaderValue.IsFinalBlock, ref _initialReaderState, ref _state);


I guess you were getting compilation errors here that "forced" you to create fields. I think the proper solution would be remove the creation of the new DdbReader struct and instead reset the reader.JsonReaderValue field:

reader.JsonReaderValue = new(originalSpan[offset..], reader.JsonReaderValue.IsFinalBlock, initialReaderState); reader.State.BufferStart += offset; reader.State.BufferLength -= offset;

firenero · 2024-05-25T18:36:05Z

src/EfficientDynamoDb/Converters/DdbConverter.cs

-                ref initialReaderState, ref reader.State);
-
+            var span = originalSpan[offset..];
+            reader = new DdbReader(span, reader.JsonReaderValue.IsFinalBlock, initialReaderState, reader.State);


Similarly to another case, it's better to reset the reader.JsonReaderValue field instead of recreating the whole DdbReader struct.

firenero · 2024-05-25T18:36:59Z

src/EfficientDynamoDb/Converters/DdbReader.cs

+            State = readStack;
+        }
+
+        internal DdbReader(ReadOnlySpan<byte> buffer, bool isFinalBlock, JsonReaderState readerState, DdbEntityReadStack readStack)


After previous changes, there won't be a need for constructor without ref params because it won't be used.

firenero · 2024-05-25T18:37:57Z

src/EfficientDynamoDb/DynamoDbContextMetadata.cs

@@ -118,7 +118,7 @@ private DdbConverter GetOrAddCachedConverter(Type propertyType, Type? converterT
                return converter;

            var nullableConverterType = typeof(NullableValueTypeDdbConverter<>).MakeGenericType(type);
-            return (DdbConverter) Activator.CreateInstance(nullableConverterType, converter);
+            return Activator.CreateInstance(nullableConverterType, converter) as DdbConverter ?? throw new DdbException("Can't create nullable value type converter.");


Just for my understanding, is there any other reason for this change except having a nicer exception message?

devtekve added 3 commits May 11, 2024 15:05

Update projects to target .NET 8.0

01d820f

This commit removes the targeting of .NET 7.0 and 6.0 in all projects, and updates them to target .NET 8.0 only. It also updates the versions of package references according to this change.

Cleaning up and undoing the whitespaces changes as they are meaningle…

3091c7c

…ss (we should maybe format it once, as it is inconsistent, but doesn't really matter much)

Since we target only .net8, this is simpler and no need to do a frame…

a02be6a

…work check

devtekve commented May 11, 2024

View reviewed changes

firenero reviewed May 25, 2024

View reviewed changes

firenero mentioned this pull request Jun 8, 2024

Native AOT #245

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade .NET version and harmonize versions of referenced packages #237

Upgrade .NET version and harmonize versions of referenced packages #237

devtekve commented May 10, 2024

firenero commented May 11, 2024

devtekve commented May 11, 2024

devtekve May 11, 2024

firenero May 25, 2024

devtekve commented May 11, 2024

firenero May 25, 2024

firenero May 25, 2024

firenero May 25, 2024

firenero May 25, 2024

firenero May 25, 2024

firenero May 25, 2024

		private JsonReaderState _initialReaderState;
		private DdbEntityReadStack _state;

Upgrade .NET version and harmonize versions of referenced packages #237

Are you sure you want to change the base?

Upgrade .NET version and harmonize versions of referenced packages #237

Conversation

devtekve commented May 10, 2024

Benchmark

.net7.0

.net8.0

Note:

firenero commented May 11, 2024

devtekve commented May 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devtekve commented May 11, 2024

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