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

.NET 6.0 #2388

Merged
merged 25 commits into from
Nov 8, 2021
Merged

.NET 6.0 #2388

merged 25 commits into from
Nov 8, 2021

Conversation

JustArchi
Copy link
Member

@JustArchi JustArchi commented Jul 27, 2021

Preview PR for the brave

Blockers for now:

  • Update csprojs
  • Update dockerfiles
  • Update CI scripts
  • Update cc.sh
  • Address new warnings
  • Update trimming settings
  • Read all available data in CryptoStream.Read() SteamRE/SteamKit#1006
  • Wait for official release
  • Remove no-longer-needed include-prerelease
  • Bump ASF to at least V5.2, evaluate breaking changes for V6.0 (unlikely)
  • Read release notes once available and correct anything critical that comes up

Wishlist for later (in the main branch once we get it merged):

  • Add osx-arm64 variant Apple Silicon Support? #2408.
  • Evaluate usage of PeriodicTimer where we currently use Timer
  • Evaluate removal of chmod +x due to ZipFile changes
  • Remove static readonly Random Random from Core/Utilities.cs and replace its uses with System.Random.Shared, which already provides a thread-safe singleton instance, enabling us to remove our lock statements completely.
  • Core.OS currently uses Process.GetCurrentProcess().MainModule?.FileName to get the processes file name and provide it to other parts of the assembly. Apart from the fact that the Process-instance is never being disposed, we will be able to replace all uses of our static readonly string with Environment.ProcessPath.
  • Evaluate, whether replacing our call to HttpHeaders.Add(...) with HttpHeaders.TryAddWithoutValidation(...) in Web/WebBrowser makes any sense.
  • Make use of CollectionsMarshal.GetValueRefOrAddDefault(...) in numerous places. This call can be used to shorten various statements like classIDsToGive[ourItem] = classIDsToGive.TryGetValue(ourItem, out uint ourGivenAmount) ? ourGivenAmount + 1 : 1; to CollectionsMarshal.GetValueRefOrAddDefault(classIDsToGive, ourItem, out _)++;, yielding a significant performance increase (52% of previous execution time for that line of code). This can be used in multiple places each in Core/Statistics and Steam/Exchange/Trading.
  • Evaluate whether we can make use of IEnumerable.TryGetNonEnumeratedCount(...).
  • C# 10 const string interpolation.
  • Consider switching to HTTP/3 if possible for HttpClient.
  • EnableCompressionInSingleFile=true if possible, should help with OS-specific builds size.
  • Add a mention that WebProxy now supports socks5://, both on wiki and in release.
  • Mention DOTNET_TieredPGO=1, DOTNET_TC_QuickJitForLoops=1 and DOTNET_ReadyToRun=0 in performance section.
  • Other non-critical code improvements based on release notes and likewise
  • Evaluate a possibility of Android/iOS build of ASF for .NET 6.0+ #2369

@JustArchi JustArchi added ✨ Enhancement Issues marked with this label indicate further enhancements to the program, such as new features. ⛔ On hold Issues marked with this label are blocked from being worked on, very often due to third-parties. 🚧 Work in progress Issues marked with this label are in active work-in-progress and they're not ready for review yet. labels Jul 27, 2021
At least runtime is no longer needed for our STD plugin, not sure about the dictionary
@JustArchi JustArchi added 🔧 Fixes required Issues marked with this label require further follow-up fixes before they can be considered. ☝️ External Issues marked with this label have external scope and typically depend on third-party projects. labels Jul 27, 2021
@JustArchi
Copy link
Member Author

AES decryption is broken almost entirely without SteamRE/SteamKit#1006 in place.

It seems plausible to wait for .NET 6.0 and SK2 fix (or confirmation that it's no longer required) at this point.

@JustArchi
Copy link
Member Author

SK2 fixed the problem (hooray), so I can keep testing it on the side.

@JustArchi JustArchi removed 🔧 Fixes required Issues marked with this label require further follow-up fixes before they can be considered. ☝️ External Issues marked with this label have external scope and typically depend on third-party projects. labels Jul 31, 2021
# Conflicts:
#	.github/appveyor.yml
#	Directory.Build.props
#	Dockerfile
#	Dockerfile.Service
@Abrynos
Copy link
Member

Abrynos commented Aug 11, 2021

With .NET 6 Preview 7 System.IO.Compression.ZipFile now respects unix file permissions. This change might come in handy during our update procedure. (reference)

@JustArchi
Copy link
Member Author

Nice, will take into account after the merge.

@Abrynos
Copy link
Member

Abrynos commented Aug 17, 2021

Several more improvement ideas:

  • Remove static readonly Random Random from Core/Utilities.cs and replace its uses with System.Random.Shared, which already provides a thread-safe singleton instance, enabling us to remove our lock statements completely.
  • Core.OS currently uses Process.GetCurrentProcess().MainModule?.FileName to get the processes file name and provide it to other parts of the assembly. Apart from the fact that the Process-instance is never being disposed, we will be able to replace all uses of our static readonly string with Environment.ProcessPath.
  • Evaluate, whether replacing our call to HttpHeaders.Add(...) with HttpHeaders.TryAddWithoutValidation(...) in Web/WebBrowser makes any sense.
  • Make use of CollectionsMarshal.GetValueRefOrAddDefault(...) in numerous places. This call can be used to shorten various statements like classIDsToGive[ourItem] = classIDsToGive.TryGetValue(ourItem, out uint ourGivenAmount) ? ourGivenAmount + 1 : 1; to CollectionsMarshal.GetValueRefOrAddDefault(classIDsToGive, ourItem, out _)++;, yielding a significant performance increase (52% of previous execution time for that line of code). This can be used in multiple places each in Core/Statistics and Steam/Exchange/Trading.
  • Evaluate whether we can make use of IEnumerable.TryGetNonEnumeratedCount(...).
  • Add <InvariantGlobalization>true</InvariantGlobalization> to our *.csproj files (or rather Directory.Build.props). This might yield another improvement due to the trimmer being able to rewrite stuff and possibly removing additional types or methods.

For details see this blog post (pretty long - use your browsers search function)

@JustArchi
Copy link
Member Author

Add true to our *.csproj files (or rather Directory.Build.props). This might yield another improvement due to the trimmer being able to rewrite stuff and possibly removing additional types or methods.

No, this breaks support for other languages than default en.

Everything else seems rational.

@JustArchi JustArchi mentioned this pull request Aug 27, 2021
7 tasks
@Abrynos
Copy link
Member

Abrynos commented Sep 27, 2021

As .NET 6.0 implements C# 10 we will be able to use const interpolated strings. This would make strings like in SharedInfo a bit nicer to look at.

internal const string ProjectURL = "https://github.com/" + GithubRepo;
internal const string ProjectURL = $"https://github.com/{GithubRepo}";

@JustArchi
Copy link
Member Author

Adding to the list:

  • Consider switching to HTTP/3 if possible for HttpClient.
  • EnableCompressionInSingleFile=true if possible, should help with OS-specific builds size.
  • Add a mention that WebProxy now supports socks5://, both on wiki and in release.
  • Mention DOTNET_TieredPGO=1, DOTNET_TC_QuickJitForLoops=1 and DOTNET_ReadyToRun=0 in performance section.

@JustArchi JustArchi removed the ⛔ On hold Issues marked with this label are blocked from being worked on, very often due to third-parties. label Nov 8, 2021
@JustArchi JustArchi requested review from ezhevita and Abrynos November 8, 2021 20:45
@JustArchi JustArchi added 🏁 Finished Issues marked with this label were finished already and no further work is required on them. 📢 Feedback welcome Issues marked with this label are open to any potential feedback that could help us. and removed 🚧 Work in progress Issues marked with this label are in active work-in-progress and they're not ready for review yet. labels Nov 8, 2021
@JustArchi JustArchi marked this pull request as ready for review November 8, 2021 20:45
@JustArchi
Copy link
Member Author

Bump in the main repo afterwards, microsoft servers are unstable today and I don't want more failed CIs to retry.

@JustArchi JustArchi merged commit 0eee213 into main Nov 8, 2021
@JustArchi JustArchi deleted the net6.0 branch November 8, 2021 22:41
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
✨ Enhancement Issues marked with this label indicate further enhancements to the program, such as new features. 📢 Feedback welcome Issues marked with this label are open to any potential feedback that could help us. 🏁 Finished Issues marked with this label were finished already and no further work is required on them.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants