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

Replace J2N.Numerics.BitOperation.TripleShift with >>> in C# 11 #115

Closed
NightOwl888 opened this issue Nov 3, 2024 · 2 comments · Fixed by #116
Closed

Replace J2N.Numerics.BitOperation.TripleShift with >>> in C# 11 #115

NightOwl888 opened this issue Nov 3, 2024 · 2 comments · Fixed by #116
Labels
good first issue Good for newcomers is:enhancement New feature or request pri:normal up for grabs This issue is open to be worked on by anyone
Milestone

Comments

@NightOwl888
Copy link
Owner

Original report: apache/lucenenet#1006

Task description

There are various places throughout the codebase where we are using the TripleShift extension method from J2N to perform an unsigned right shift, since older versions of C# did not have this operator. C# 11 added the >>> operator like Java has to do this, which gets translated directly to the shr.un opcode, so should be more efficient than the several opcodes involved in the various TripleShift overloads, even if it is aggressively inlined. In addition, it will help make direct porting from Java easier.

(Note: the aggressive inlining doesn't seem to be working in our published NuGet packages, as spot checking one example in BaseCharFilter still had a call operation to TripleShift, which is certainly going to be worse for performance than a single opcode.)

I have confirmed this works on net462, our oldest target, with LangVersion set to 11, for all overloads that TripleShift supports.

Need to confirm that setting our LangVersion to 11 does not cause regressions elsewhere.

We should not be calling TripleShift() internally in J2N, however to avoid breaking API changes, we should leave these methods intact without changing them to >>>.

@paulirwin
Copy link
Collaborator

This could be broken out separately, but at some point TripleShift should be also marked as obsolete.

@NightOwl888
Copy link
Owner Author

This could be broken out separately, but at some point TripleShift should be also marked as obsolete.

I considered it, but there are cases where it may still be required (for example, if a project is somehow blocked from upgrading to C# 11.0).

J2N has different goals than Lucene.NET. The goal here is to fill gaps between Java and .NET and to normalize differences between different target frameworks. Since this is one of those gaps, it is still relevant.

But the goal of Lucene.NET is to remove dependencies on J2N. C# is getting closer all the time to filling in missing pieces that the JDK has. We may never get to the point where the BCL is good enough to completely remove the dependency on J2N, but each cord we cut with J2N is a win (ICharSequence is next).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers is:enhancement New feature or request pri:normal up for grabs This issue is open to be worked on by anyone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants