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

AOT compilation produces single trim analysis warning #1942

Closed
DavidZidar opened this issue May 19, 2023 · 10 comments · Fixed by #1968
Closed

AOT compilation produces single trim analysis warning #1942

DavidZidar opened this issue May 19, 2023 · 10 comments · Fixed by #1968
Assignees

Comments

@DavidZidar
Copy link

Bug

Which library version?

6.0.0

What are the platform(s), environment(s) and related component version(s)?

.NET 7 and .NET 8 preview 4

What is the use case or problem?

I am AOT-publishing a small application that is using System.Reactive and I'm getting a single trim analysis warning.

What is the expected outcome?

No warnings.

What is the actual outcome?

ILC : Trim analysis warning IL2057: System.Reactive.PlatformServices.CurrentPlatformEnlightenmentProvider.GetService<T>(Object[]): Unrecognized value passed to the parameter 'typeName' of method 'System.Type.GetType(String,Boolean)'. It's not possible to guarantee the availability of the target type.

Other than the warning it seems to work fine.

Related question

Is AOT supposed to be supported? I see that the IsTrimmable flag is set to true but the warning I get makes me uncertain.

@idg10
Copy link
Collaborator

idg10 commented May 19, 2023

The intention is that trimming is supported. That is why we added the IsTrimmable flag, and we added annotations for the parts of Rx that are inherently incompatible with trimming.

That said, we did most of the testing for trimming against Blazor scenarios in which assemblies were being trimmed but not AOT-compiled.

I'd like to investigate this problem, so could you tell me what kind of app this is and what is the target platform?

@DavidZidar
Copy link
Author

DavidZidar commented May 19, 2023

That's great to hear! In my case it's a regular console application on Windows.

I managed to reproduce the issue like this.

ConsoleApp.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net7.0</TargetFramework>
    <RootNamespace>ConsoleApp</RootNamespace>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <PropertyGroup>
    <PublishAot>true</PublishAot>
    <TrimmerSingleWarn>false</TrimmerSingleWarn>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="System.Reactive" Version="6.0.0" />
  </ItemGroup>

</Project>

Program.cs

using System.Reactive.Linq;
using System.Reactive.Subjects;

using var subject = new Subject<string>();
using var subscription = subject.Subscribe(Console.WriteLine);

Then publish:

dotnet publish

@DavidZidar
Copy link
Author

DavidZidar commented May 19, 2023

These lines seem to be the offending code, a type name is constructed using strings such that the IL-compiler can't know if the type has been trimmed away or not.

var asm = new AssemblyName(ifType.Assembly.FullName!)
{
Name = "System.Reactive"
};
var name = "System.Reactive.Linq.QueryDebugger, " + asm.FullName;
var dbg = Type.GetType(name, false);
if (dbg != null)
{
return (T?)Activator.CreateInstance(dbg);
}

Maybe that code can be replaced with new QueryDebugger() instead?

@idg10
Copy link
Collaborator

idg10 commented Jun 14, 2023

The Platform Enlightenment Provider system is tricky, so this needs some thought. It's worth talking through why it exists, because there are some backwards compatibility constraints on changing it.

Platform Enlightenments were introduced in Rx 2.0. The only documentation I've been able to find is the Rx 2.0 beta announcment (only available on the Wayback Machine because this was one of many articles posted to blogs.msdn.com that was lost when Microsoft decided to shake up their blog engine a few years back and apparently felt free to delete enormous numbers of old posts).

This feature was introduced to enable most of Rx to become platform independent. Back in 2012, there were quite a few ever slightly so different versions of .NET (including Silverlight, Windows Phone, Windows 8 store apps, XNA, XBox). NuGet was still relatively new, and not yet that well established, so Rx was available both via NuGet but also as an SDK installable with an MSI. And there no such thing as .NET Standard. Portable Class Libraries had only just been introduced, and Rx 2.0 took advantage of this, so that they didn't have to build and distribute different versions for every supported platform. Platform Enlightenments were added as part of the work to be able to build a PCL version of Rx that would run on a range of platforms.

One of the main problems Rx faced at the time when it came to building a portable library was that support for threading varied wildly across different platforms. Many of the supported flavours of .NET did not support the direct creation of new threads. (In the case of the versions supporting touch screen devices, the concern at the time was that if applications could freely create threads, this could have system-wide implications for performance and battery life.) The thread pool features varied somewhat by platform too.

This was an issue for Rx because some schedulers typically need to be able to exercise control over threads. The problem with having a single portable Rx assembly was that if you have access only to the functionality that was available across all the supported .NET targets at the time, the result would be a lowest common denominator solution that would be suboptimal on some platforms.

So the idea of Platform Enlightenments was introduced. It was inspired by an idea from the world of Virtual Machines. When VMs were first introduced, guest operating systems had no idea they were being virtualized. The host had to emulate things like disk controllers, network cards, and graphics cards, so that the device drivers in the guest OS would think they were talking directly to real hardware. This was pretty inefficient, and over time, mechanisms were added to enable a guest to discover that it was a VM, and to use more efficient VM-native mechanisms. The old approach still worked—you could happily use an OS that was oblivious to virtualization. But if you had a guest OS that was enlightened about such things it would perform better.

And so in Rx 2.0, the idea was that the core libraries were able to detect when they were running on certain known environments, and use scheduler implementations optimized for the platform on which they were running. This was the Platform Enlightements concept. The basic idea was that everyone used the same portable core Rx libraries, but those would look on disk to see if a Platform Enlightenments DLL was present, and if it was, they would load that and use that to ensure the best possible performance on the platform in question. (But there was a lowest common denominator implementation built into the core that it could fall back to, IIRC.)

Now let's return to the present day, and the .NET landscape has changed. PCLs effectively became irrelevant once .NET Standard came along. But more importantly for this discussion, there is much less difference in threading functionality between the various different platforms Rx supports. The seismic shift here was .NET Standard 2.0, and its requirement that compatible platforms provide full multithreading support. By this time, the old Windows-Phone-only targets were no longer relevant, and the successor to the Windows 8 era touch screen platform was UWP. The initial versions of UWP did not provide a full complement of threading services, but once UWP added support for .NET Standard 2.0, which happened in the October 2017 release of Windows 10 (build 16299, aka "Fall Creators Update") Rx no longer really had any need for Platform Enlightenments: the basic cross-platform portable functionality was, finally, good enough.

However, Rx couldn't simply remove the Platform Enlightenments system.

Enterprising developers had found other ways to use this. Although Platform Enlightments had been designed purely to enable cross-platform code to enjoy platform-specific optimizations, they effectively provided a way to intercept various operations in Rx, and people had used this either to impose their own threading constraints, or to provide monitoring, debugging, and, according to the comment inside the PlatformEnlightenmentProvider class, even features such as quota management and policy enforcement!

So although the PlatformEnlightenmentProvider no longer searches in the local folder to find an enlightenment provider DLL—it just new()s up a CurrentPlatformEnlightenmentProvider—the PlatformEnlightenmentProvider.Current property remains writeable to ensure backwards compatibility with code that was exploiting this (technically unsupported) capability.

Since the code that goes looking on disk for an enlightenment provider has gone, I was slightly surprised by the problem you've found—I thought Rx had stopped doing this years ago.

But this looks like a remnant of the old way of doing things. It's specific to debug scenarios, which seems to be why this is off in its own little bit of code (and therefore didn't get removed when other similar enlightment-based code stopped doing dynamic loads). And the reason for that seems to be that Rx only ever wants to run this code path when a debugger is attached.

Looking at the comment, you can see that this code was written on the expectation that the type it wants to return—QueryDebugger—will live in a separate assembly that needs to be loaded dynamically. But if you look at what the code actually does, it seems always to end up looking in the System.Reactive component (i.e., the exact same component that contains this code).

There was a time when that wasn't true. In the past, this used to look for a component called System.Reactive.Debugger. But I'm not sure if any such component ever existed. This commit:

40e8696

changed it always to look in System.Reactive, but that means that this is now an unnecessarily roundabout way of loading a type!

Prior to that commit, removing the dynamic loading would, strictly, have broken backwards compatibility, because it would mean that should a System.Reactive.Debugger component exist, the current code would load it, but changing to using new() would not.

However, that commit, which was made about 6 years ago, would already have broken anyone relying on the dynamic loading of a System.Reactive.Debugger component. Nobody has complained yet, so I think we're probably safe... (And this only affects processes with a debugger attached.)

So in conclusion, I think that it is in fact safe to make the change you suggest.

I shall now do that.

@idg10 idg10 self-assigned this Jun 14, 2023
@idg10 idg10 added the bug label Jun 14, 2023
@idg10 idg10 linked a pull request Jun 14, 2023 that will close this issue
@idg10
Copy link
Collaborator

idg10 commented Jun 14, 2023

I've pushed a test release to the Rx preview package feed: https://dev.azure.com/dotnet/Rx.NET/_artifacts/feed/RxNet/NuGet/System.Reactive/overview/6.0.1-preview.1.g5a437431d2

Using the 6.0.0 RTM build, I was able to repro the problem based on your example. Upgrading to this preview build with the fix seems to resolve the problem.

You could give that a try now if you want to verify that the fix works for you. Or if you don't want to have to set up your system to point at the preview feed, we'll do a preview release to NuGet once we've completed review of the PR and merged it.

@DavidZidar
Copy link
Author

Great writeup! I just tried the preview package and can confirm the warning disappeared on my end as well.

@idg10
Copy link
Collaborator

idg10 commented Jun 14, 2023

OK, that's now up on NuGet as System.Reactive 6.0.1-preview.1.

In general, I prefer not to go too quickly from preview build to corresponding release build in case we discover that actually we've not fixed the problem yet. And I don't believe this fix is urgently widely needed, because the relevant code only ever runs when you have a debugger attached.

So my current plan is to give it a week or so before we do a non-preview release. However, if that's going to cause you problems, please let me know.

@DavidZidar
Copy link
Author

Sounds good to me!

@JensNordenbro
Copy link

Is the intention also to create net7.0 and net8.0 monkier builds for the NuGet:s ?

@idg10
Copy link
Collaborator

idg10 commented May 4, 2024

Is the intention also to create net7.0 and net8.0 monkier builds for the NuGet:s ?

We only add new targets when:

  1. We can usefully make the code different (e.g. if one of these runtimes were to offer new functionality that Rx could make use of)
  2. One of the existing targets goes out of support (e.g. we added a net6.0 target when .NET 5.0 went out of support)

The only reason we do 2 is because we don't support Rx on unsupported versions of .NET. .NET 6, 7, and 8 are all perfectly happy to load components with a net5.0 TFM. But since we don't run tests on .NET 5.0, we bumped the TFM to the oldest version of .NET still in support.

So we will release a new versioning targeting .net8.0 when .NET 6.0 goes out of support.

We support .NET 8.0 and have done for months - we added it to the unit test suite months ago. But since .NET 8.0 has no new features that Rx could usefully exploit it would have been pointless to add a separate net8.0 TFM. It would gave contained exactly the same code as the net6.0 one.

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

Successfully merging a pull request may close this issue.

3 participants