Skip to content

Conversation

Gitii
Copy link

@Gitii Gitii commented Sep 21, 2024

Hi,

I wanted to share my proof of concept for aot support.
Under Windows, Hardware.Info uses WMI to gather hardware details. COM is used to interface with WMI. The "old" COM intertop in .net doesn't support aot compilation. Luckily com wrappers have been introduced which "provide an optimized and AOT-friendly COM interop solution".

This proof of concept is not ready to be merged as-is but shows that it's possible to to have both - WMI and aot.

Changes

  • interface IHardwareInfoRetrieval and class HardwareInfoBase are now public
  • class HardwareInfo has a new constructor which accepts an instance of IHardwareInfoRetrieval
  • Added new library Hardware.Info.Windows.Aot which implements class HardwareInfoRetrievalWindowsAot
  • Added copy of Hardware.Info.TestAot which enables aot publication and uses Hardware.Info.Windows.Aot

My goal has been to compare both implementations (old COM interop and new COM wrappers). Aot support should be a drop-in solution and not require any other changes. Therefore I made interface IHardwareInfoRetrieval and class HardwareInfoBase are public.

Next Steps

If aot support is a viable path forward, I suggest that

  • Aot support is provided as separate pre-release package - early adapters can opt-in and share feedback. I am not a aot expert and there might be some quirks (see section about current issues for more details)
  • Move current windows implementation into a separate library. Hardware.Info still references non-aot compatible packages. I think that's fine as long as the code paths aren't used - they might actually be trimmed out when trimming is enabled. But this should be investigated to prevent issues for users. Moving the windows implementation out into a separate library would avoid all these issues but that's a breaking change.
  • The code still needs some clean up and refactoring - it's just a proof of concept after all.
  • Check how System.Management initializes and un-initializes com (see issues section).
  • Some research how to properly mark a package as aot compatible and trimmable.
  • Better error handling, no more Console.WriteLine

Issues

  • The library still needs to initialize com. As far as I understand, this can (should?) only be done once. How this could be checked - I do not know
  • Currently com is un-initialized on clean up - this might no be desired.
  • This is not related to oat but I've noticed that several big numeric properties (like filesystem size) could cause an integer overflow. WMI exposes these fields as strings and they are parsed into (long) integers. System.Numerics.BigInteger might be a better alternative - but that's a breaking change.

@Jinjinov
Copy link
Owner

Jinjinov commented Sep 21, 2024

Hi!

Thank you for your interest in contributing to Hardware.Info

In a few days I am traveling to Japan for one month, so I don't have the time to check the code changes.

I know very little about AOT, just that Microsoft is trying hard to make it possible, so your suggestions sounds reasonable.

A few remarks:

  • Hardware.Info will remain a .NET Standard 2.0 library to remain compatible with .NET Framework 4.8
  • it will remain one library for all platforms, for backwards compatibility
  • if AOT can't be added in the same NuGet, it will probably be in Hardware.Info.Aot except if there is a good reason for another approach
  • case is debatable, Aot vs AOT, but a quick NuGet search shows more AOT packages
  • Hardware.Info.Aot.Test looks better than Hardware.Info.TestAot
  • I would avoid making interface IHardwareInfoRetrieval and class HardwareInfoBase public at all costs and only do it as a last resort.
    I would avoid adding IHardwareInfoRetrieval as a constructor parameter at all costs and only do it as a last resort.
    One solution could be inheritance of IHardwareInfo in the Aot NuGet, if it makes sense.
    Another solution could be Friend assemblies: https://learn.microsoft.com/en-us/dotnet/standard/assembly/friend
    A third solution could be DI with a Configure extension method
    If making them public is the only solution, I would rename it to something better, perhaps IPlatformHardwareInfo.
    HardwareInfoBase can be refactored to a helper class, either static or with DI or with composition - to avoid making it public.

It looks like Microsoft.Extensions.DependencyInjection is .NET Standard 2.0 compatible, but I would have to check if using it is a breaking change or not.

Is this one task or two? :)

Better error handling, no more Console.WriteLine

There is no error handing with Console.WriteLine :)
I just checked, Microsoft.Extensions.Logging is .NET Standard 2.0 compatible, so that would be a nice change.

Could you explain about System.Numerics.BigInteger in more detail, with concrete properties? What are the chances of this actually being a problem? Why would it be a breaking change? I actually don't mind making this change, even if it is a breaking change... unless it would actually break existing programs, but I don't see how?

Please know that I wrote all of this very fast, so if something doesn't make sense, just point it out :)

@Gitii
Copy link
Author

Gitii commented Sep 21, 2024

Hi @Jinjinov,

thank you for your fast response and no pressure. This is just a proof of concept and not something that is ready to be merged.
Focus on your travel and have a nice month in Japan!

Your remarks sound reasonable, and I am willing to work on aot support for Hardware.Info (slow and steady, I am doing this in my free time), if you need support.
I am not sure if .net standard 2.0 (as being a specification) would support aot which is really a .net 6+ thing. The old .net framework will never support ahead of time compilation (it has it's own unique advantages with ngen and jit).
From an aot perspective, .net 6 and newer are the only supported versions.
A separate library would allow users to opt-in on newer platforms while existing users can use current solution.

Regarding System.Numerics.BigInteger:
I double checked and I think I copied something wrong. The data types are correct. To my defence, it was late when I wrote this code.

My suggestion is that you do your travel first and then take a look at the code - when you have time.
This POC is not time critical, and you should not feel pressured to look into this when you are traveling.

@Jinjinov
Copy link
Owner

Ok, I agree.

I made a new git branch aot so I can merge your PRs and still keep the master branch, so your PR should target aot branch.

Here are a few suggestions:

  • split the changes into several smaller PRs (they can depend on one another if needed, you can also create new branches) so I won't be overwhelmed
  • the new projects should be Hardware.Info.Aot, Hardware.Info.Aot.Windows and Hardware.Info.Aot.Test, all for .NET 8
  • rename IHardwareInfoRetrieval to IPlatformHardwareInfo
  • rename class HardwareInfoBase to class PlatformHardwareInfoBase
  • rename class HardwareInfoRetrievalWindowsAot to class PlatformHardwareInfo in Hardware.Info.Aot.Windows project
  • there is actually no need to inherit HardwareInfo in Hardware.Info.Aot or to use DI - to keep it simple, Hardware.Info project can be split into Hardware.Info.Core (includes Components, IHardwareInfo, IPlatformHardwareInfo, PlatformHardwareInfoBase), Hardware.Info.Linux, Hardware.Info.Mac, Hardware.Info.Windows projects and then include them
  • Hardware.Info.Aot includes Hardware.Info.Aot.Windows instead Hardware.Info.Windows
  • the current HardwareInfo class is renamed to HardwareInfoBase and included in Hardware.Info.Core
  • the code in the constructor of current HardwareInfo class can be moved to a virtual method Setup or Initialize
  • Hardware.Info and Hardware.Info.Aot NuGet packages only define the HardwareInfo class which inherits the new HardwareInfoBase and only overrides the new Setup / Initialize

This way, all public interfaces and classes stay the same, so there are no breaking changes.

And there is no need:

  • for Hardware.Info.Aot NuGet to include Hardware.Info NuGet, which is great
  • for the interface to become a constructor parameter, which is the main thing I wanted to avoid

I still have not decided if internal should become public (always risky, because any change to classes that were internal could become a breaking change because users of the NuGet can now access and use everything) or if internal should stay internal and the new projects should use

[assembly: InternalsVisibleTo("Hardware.Info")]
[assembly: InternalsVisibleTo("Hardware.Info.Aot")]

I would wait with Microsoft.Extensions.DependencyInjection and Microsoft.Extensions.Logging for now.

I know that this is a lot of work, but as you said, there is no rush.

I can help when I come back. I also do this in my free time after work. Out of all my C# projects, I spend the most time working on https://github.com/Jinjinov/OpenHabitTracker - but I will take some time for this too.

Feel free to comment or point out anything that doesn't make sense :)

@Gitii
Copy link
Author

Gitii commented Sep 22, 2024

@Jinjinov
Using InternalsVisibleTo to keep internals private sounds reasonable.
I agree that DI increases complexity with little benefits in this scenario. I only mentioned logging because there are some cluttered logging statements instead of proper error handling. Throwing an exception is better but was out of scope for this proof of concept.

Is this the package structure that you have in mind?
I tried to sketch it according to your requirements.
hardware_info

It might be possible to include Hardware.Info.Core as part of Hardware.Info.* instead of shipping it through nuget - assuming that my project structure is correct.

@Jinjinov
Copy link
Owner

Oh, you added Console.WriteLine in your PR... I misunderstood...

Yes, this is the package structure I had in mind. Only 2 lines are missing: Hardware.Info.Aot should also include Hardware.Info.Linux and Hardware.Info.Mac - assuming this is possible? I really want the NuGet package to be as simple as possible: includes all platforms and needs no configuration.

Only Hardware.Info and Hardware.Info.Aot are NuGet packages, all others are just library projects referenced in the NuGet packages.

@Gitii
Copy link
Author

Gitii commented Sep 22, 2024

hardware_info2

Like this?

@Jinjinov
Copy link
Owner

Yes, if this is possible, this would be perfect :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants