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

Drop .NET Standard 2.0 Target #1041

Open
1 task done
paulirwin opened this issue Nov 20, 2024 · 9 comments
Open
1 task done

Drop .NET Standard 2.0 Target #1041

paulirwin opened this issue Nov 20, 2024 · 9 comments
Assignees
Labels
is:task A chore to be done pri:normal
Milestone

Comments

@paulirwin
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Task description

See #1040 for context. Given the performance penalty for using the netstandard2.0 target, and lack of currently-supported .NET implementations that support it but are not otherwise covered by our other targets, we should drop the netstandard2.0 target. This can be done independently of dropping netstandard2.1, which might remain around for Unity support.

Any libraries that depend on Lucene.NET via netstandard2.0 are encouraged to target net462 and net8.0 (at this time) for better performance. .NET Standard 2.0 lacks performance-optimized APIs and thus causes a performance penalty compared to other targets if you use Lucene.NET in this way.

@paulirwin paulirwin added the is:task A chore to be done label Nov 20, 2024
@paulirwin paulirwin added this to the 4.8.0-beta00018 milestone Nov 20, 2024
@paulirwin paulirwin self-assigned this Dec 3, 2024
paulirwin added a commit to paulirwin/lucene.net that referenced this issue Dec 3, 2024
paulirwin added a commit to paulirwin/lucene.net that referenced this issue Dec 3, 2024
@paulirwin
Copy link
Contributor Author

I have this change ready to PR, as soon as #1052 is merged.

@jeme
Copy link
Contributor

jeme commented Dec 3, 2024

I am curious to what performance penalty a target that simply has no implementation has?.
Remember, this is just a contract target, .netstandard 2.0 is not an implementation. It would have to mean we have cases where we say "If it's .NET 4.6.1 then A or if Standard 2.0 then B" - But do we have allot of that around?

The .netstandard 2.0 target greatly simplifies things for framework developers that develops around lucene.

Before we "Claim" any performance penalty, could we please prove those?

https://learn.microsoft.com/en-us/dotnet/standard/net-standard?tabs=net-standard-2-0#net-standard-not-deprecated

@paulirwin
Copy link
Contributor Author

The .netstandard 2.0 target greatly simplifies things for framework developers that develops around lucene.

By "framework developers" do you mean developers of frameworks-as-libraries that depend on Lucene.NET, or ".NET Framework" developers? I suppose both apply, although note that we are still going to have our net462 build for .NET Framework users.

I'll let @NightOwl888 speak to the performance, since he mentioned it at #1040. One challenge with having to claim a performance penalty is because of the difficulty of actually using the netstandard2.0 target directly. Any .NET Framework version is going to prefer our net462 build over netstandard2.0, and any out-of-support .NET (Core) 3.0-7 is going to prefer our netstandard2.1 build (and then of course, .NET 8+ would use our net8.0 build). I suppose we could create some benchmarks that transitively depend on us through a netstandard2.0 library to force that, which would emulate some real-world transitive scenarios that could cause this.

In either case, after thinking through it more, I do think we can reconsider this for post-4.8. One scenario is that users may be consolidating their shared logic that uses Lucene.NET into a netstandard2.0 class library to i.e. ease a transition to modern .NET from .NET Framework, which was common practice several years ago in the early days of modern .NET, even though they would likely be better served today by multi-targeting instead (or, better yet, finishing the transition to modern .NET).

@jeme
Copy link
Contributor

jeme commented Dec 4, 2024

By "framework developers" do you mean developers of frameworks-as-libraries that depend on Lucene.NET, or ".NET Framework" developers? I suppose both apply, although note that we are still going to have our net462 build for .NET Framework users.

This only concerns developers that creates frameworks around Lucene, who wishes to target both a .NET 4.X and .NET 8+ Audience in a convinient way (Perhaps often in an early stage of a project). And to this date, using .netstandard 2.0 is still a thing to bridge the gap between .NET Framwork and .NET 8+ because it's still far from all tha has moved over. It's not something that was done years ago, it still happens because big businesses move slow or big solutions are not cheap to migrate but eventually I think teams does have plans to do so (I know we are still stuck on .NET 4.8 on projects and actively preparing hundred thounsand lines of code for the move, and .NET Standard is very usefull in that process. But the problem is, it's allot of work, someone has to pay etc. etc.)

Interestingly enough, Taken from one .NET Framework 4.8 Web application we have going, which targets a package that is developed against .netstandard 2.0 that uses Lucene, the target keeps the package in the middle easy to develop and allows it to be used from both .NET Framework AND .NET 8+. It's however not the .netstandard 2.0 version of lucene that ends up in the final build. So the package in the middle gains the convinience here but for .NET Framwork does not suffer any penalties (If there is any). I am not sure if the same thing would happen for .NET 8+ but it's an interesting observation.

Skimming compiler flags etc, I am having a very hard time figuring out what the differences is between the .NET 462 and .NET Standard 2.0 Build, And therefor where the performance penalty between the two are.

If there is not performance gains in targeting .NET 462 over .NET Standard 2.0, then I would actually advocate that it would be better to throw out the .NET 462 target.

@paulirwin paulirwin modified the milestones: 4.8.0-beta00018, Future Dec 4, 2024
@paulirwin
Copy link
Contributor Author

I'll let @NightOwl888 chime in on this one, but for now I think there's clear enough justification to punt on this regardless of performance. Moving to the Future milestone, but keeping open for discussion and to indicate that we do hope to drop it someday in a future version of Lucene.NET.

The idea of dropping the net462 target while keeping netstandard2.0 is an interesting one that I think is worth consideration as well. That would let us just run tests on .NET Framework once for the netstandard2.0 build.

@NightOwl888
Copy link
Contributor

Before we "Claim" any performance penalty, could we please prove those?

To qualify this, I am referring to the original conversation when we were talking about removing netstandard2.1.

First of all, netstandard2.0 shipped before there was a GetEnumerator() or AddOrUpdate() method on ConditionalWeakTable<TKey, TValue>. For GetEnumerator() we worked around by using Prism weak events (which we recently added to the Support namespace) and for AddOrUpdate(), we are using a Remove() followed by Add(). During experimentation, it was clear that using WeakReference (which is how the Prism weak events are implemented) holds onto references longer than using purely a ConditionalWeakTable<TKey, TValue>. However, there is a WeakEventManager (from WPF) that exists only on .NET Framework that would probably perform better that uses another ConditionalWeakTable<TKey, TValue> along with some native code under the hood. The Prism weak events solution we have is the best we can do for netstandard2.0 without dropping to native code. Remove() followed by Add() is also less efficient than AddOrUpdate().

Note that the netstandard2.1 target primarily exists because of the missing APIs on ConditionalWeakTable<TKey, TValue>.

Secondly, and more importantly, when the netstandard2.0 target is chosen, all of its dependencies can only target netstandard2.0 or lower. If the runtime is net8.0 or net9.0 and Lucene.Net is netstandard2.0, it will no longer call the hardware intrinsics APIs that didn't exist until net6.0 because it will load System.Memory and J2N assemblies that target netstandard2.0. System.Memory and J2N both use hardware intrinsics only if the target is .NET Core. And this goes well beyond those two dependencies.

As for the difference between net462 and netstandard2.0, the performance advantage is not as significant in that case. Although, there are still a few optimizations with having a native runtime target over a .NET Standard target.

Do I think that we should drop netstandard2.0? Not at this time. But I think we should consider doing it before we drop netstandard2.1 because of the missing APIs on ConditionalWeakTable<TKey, TValue>. However, as long as Microsoft is still targeting netstandard2.0 it is a strong indicator that we should be, as well.

That being said, I think we should actively discourage anyone from using netstandard2.0 as a way to target both .NET Framework and .NET Core, because doing so loses several of the performance advantages of using .NET Core (at least for our class libraries and any class libraries we depend on). That is, unless you are swapping in the .NET Core assembly into the final build (although, there may be some minor API differences where you could get burned by doing that).

@jeme
Copy link
Contributor

jeme commented Dec 4, 2024

Secondly, and more importantly, when the netstandard2.0 target is chosen, all of its dependencies can only target netstandard2.0 or lower. If the runtime is net8.0 or net9.0 and Lucene.Net is netstandard2.0, it will no longer call the hardware intrinsics APIs that didn't exist until net6.0 because it will load System.Memory and J2N assemblies that target netstandard2.0. System.Memory and J2N both use hardware intrinsics only if the target is .NET Core. And this goes well beyond those two dependencies.

As for the difference between net462 and netstandard2.0, the performance advantage is not as significant in that case. Although, there are still a few optimizations with having a native runtime target over a .NET Standard target.

Do I think that we should drop netstandard2.0? Not at this time. But I think we should consider doing it before we drop netstandard2.1 because of the missing APIs on ConditionalWeakTable<TKey, TValue>. However, as long as Microsoft is still targeting netstandard2.0 it is a strong indicator that we should be, as well.

That being said, I think we should actively discourage anyone from using netstandard2.0 as a way to target both .NET Framework and .NET Core, because doing so loses several of the performance advantages of using .NET Core (at least for our class libraries and any class libraries we depend on). That is, unless you are swapping in the .NET Core assembly into the final build (although, there may be some minor API differences where you could get burned by doing that).

No, this is not what happens. At least not in my testing. And I have tested both a .NET 8 and .NET 4.8 scenario.

Given I have a .NET 8 project that references a .NET Standard 2.0 Project or Package that references Lucene, I DON'T get the Lucene .NET Standard 2.0 image/dll, I get the one from .NET 8.0.

Dependency Tree:

  • ConsoleApp1 [.net8.0] > ClassLibrary1 [.netstandard2.0] > Lucene.Net 4.8.0-beta00017

Given I have a .NET 4.8 project that references a .NET Standard 2.0 Project or Package that references Lucene, I DON'T get the Lucene .NET Standard 2.0 image/dll, I get the one from .NET 4.6.2.

Dependency Tree:

  • ConsoleApp2 [.net4.8] > ClassLibrary1 [.netstandard2.0] > Lucene.Net 4.8.0-beta00017

image

I also did binary comparisons. (Well hex comparison using beyond compare). But that is technically a binary comparison just displaying the bytes in hex.

Yes, I WON'T say that I am not a little surprised my self >.<... But only a little... I would encurage you to try it out your self.

Now this raises some other serious questions about how this can break things if you used it maliciously >.<... But... Hmmm. (I Really doubt they do some huristics to ensure that the targeted one is actually truely compatible but I don't know, all I can see is that the .netstandard2.0 output from Lucene.NET does not end up in any of the end results I tested)

As far as going for testing where ClassLibrary1 would be a Nuget packages instead, I have ONLY tested the .NET 4.8 scenario (this is what we have a couple of at work) and that gives the same experience. So I ASSUME that if I where to target that package from .NET 8 I would also replicate what I see here locally.

So my conclussion for now is that it's your "ROOT" project that determines which image that gets selected, not any intermediate projects or packages.

So it seems you actually essentially don't pay any penalties. And this would seem this only occur if Lucene under .net8.0 and .net462 had a "SuperSlowAddDocument" AND a "SuperFastAddDocument" where .netstandard2.0 only had "SuperSlowAddDocument" method due to limitations in the API surfaces. Then ClassLibrary1 could not use "SuperFastAddDocument" and would have to stick with the "SuperSlowAddDocument" implementation hence you only get to use the slow version.

@NightOwl888
Copy link
Contributor

Okay, that is a bit puzzling.

If you have a completely different API that is conditionally compiled for netstandard2.0 vs net462, that would certainly break if it were the case.

public class SomeClass
{
#if NETSTANDARD2_0
    public static void SomeMethod()
    {
    }
#else
    public static void SomeMethod2()
    {
    }
#endif
}

If your ClassLibrary1 project targeting netstandard2.0 called a multi-targeted assembly with this class in it, does your call to SomeMethod() inside of ClassLibrary1 break?

@jeme
Copy link
Contributor

jeme commented Dec 4, 2024

Okay, that is a bit puzzling.

If you have a completely different API that is conditionally compiled for netstandard2.0 vs net462, that would certainly break if it were the case.

public class SomeClass
{
#if NETSTANDARD2_0
    public static void SomeMethod()
    {
    }
#else
    public static void SomeMethod2()
    {
    }
#endif
}

If your ClassLibrary1 project targeting netstandard2.0 called a multi-targeted assembly with this class in it, does your call to SomeMethod() inside of ClassLibrary1 break?

Yes, that was what I was refering to when I said it would seem that you could certainly be malicious here. (It could also happen unintentionally). Unless they do some crazy huristic scan and pick based on that but I REALLY doubt that.

It would be a bit more complicated to set up an example to test that I think and I don't feel like I have the time for that right now, although I am currious.

But if you think about the intention of NET Standard, and stick with that intention, the output targeting NET Standard SHOULD be the common subset that is shared between you NET 8 and NET 462 output (From your public API's/Contracts perspective that is, implementations can ofc differ), it should not add ADDITIONAL stuff.

Your example would violate that pritty hard.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:task A chore to be done pri:normal
Projects
None yet
Development

No branches or pull requests

3 participants