Replace all "functional interfaces" with delegates and their anonymous classes with lambdas

[paulirwin]

Is there an existing issue for this?

• I have searched the existing issues

Task description

From reviewing code as part of fixing #715. In Java 8+, you can pass a lambda for an argument that is a "functional interface" type, which is an interface with a single abstract method (SAM). This is roughly analogous to delegates in .NET, although not as powerful as delegates. In Java, lambdas are basically just a simpler way of implementing an anonymous class for a functional interface. But since Lucene at the time targeted Java 7, they could not take advantage of this feature like we can today.

Aside: Functional interfaces in modern Java should have a @FunctionalInterface annotation added, although this is not a requirement. SAM is sufficient.

We should review to see if I've missed any below, and compare to the latest Lucene code any interfaces or abstract classes that are still functional (have not had other methods added) and consider converting those to delegate types where it makes sense. They should still be named delegates, rather than i.e. Action<T>, just without the I convention for interfaces. Then, we can replace many "AnonymousClass" implementations in Lucene.NET with lambdas instead.

As an example, this entire anonymous class could be replaced with _ => true:

private sealed class PredicateAnonymousClass : IPredicate<object[]>
{
    public bool Apply(object[] args)
    {
        return true;
    }
}

Candidates:

• ITestPoint (SAM in latest Lucene) (cannot be done due to implementations in TestIndexWriterExceptions having fields)
• IReaderDisposedListener (@FunctionalInterface in latest Lucene)
• SegmentReader.ICoreDisposedListener (@FunctionalInterface in latest Lucene as ClosedListener)
• IAttributeReflector (if generic overload is moved to an extension/external method; @FunctionalInterface in latest Lucene)
• TestRandomChains.IPredicate<T> (could be replaced with .NET's built-in Predicate<T> delegate, as this was changed to use java.util.function.Predicate in latest Lucene)
• IAutomatonProvider (SAM in latest Lucene)
• IndexReaderWarmer (abstract class in Lucene.NET; @FunctionalInterface in latest Lucene) (cannot be done due to SimpleMergedSegmentWarmer)

This will set us up for porting future @FunctionalInterfaces in post-4.8 Lucene as delegates and lambdas instead of interfaces and anonymous classes.

[NightOwl888]

A word of caution here. The first stab at implementing Debugging.Assert() used Func<bool>:

• Implementation of Replacement for Debug.Assert() that can be turned on/off in Release build (addresses #326) #327

We found that using this approach was very detrimental to performance. At the time, it was presumed that this was due to the lambda capture allocations rather than the use of Func<bool> (which could be passed a method that returns bool instead) but no benchmark was done to determine which of these (or both) caused the performance degradation.

• Change Debugging.Assert usage to not allocate due to lambda capture #372
• Remove delegate based debugging assert #373

There was also an effort to factor out some hand coded classes. For example:

lucenenet/src/Lucene.Net/Search/FieldCache.cs

Line 382 in d597a5f

public Bytes(Func<int, byte> get) // LUCENENET specific - Added constructor overload to simulate anonymous classes

That effort was stopped as soon as we discovered that using lambdas was very costly, but we didn't go back and revert those to hand-coded classes, either. Maybe we wouldn't have to if we could pass it a function instead of a lambda, which may not incur the allocation overhead. Benchmarking is needed.

Giving it a similar UX as Lucene is great, but if it incurs a significant performance penalty, we should stick with the hand-coded classes.

[paulirwin]

@NightOwl888 Thanks for that context. Looking at the changes to fix the performance issues you mention, it was a difference between having a lambda that captures and a string interpolation, which is quite different than the scenario here.

When data in the surrounding scope is captured, here it's the difference between having a custom AnonymousClass that captures the data in a field, and for a lambda, that is generated by the compiler as a class that captures the field, so they should be about identical in terms of heap allocation and performance. The compiler might be able to apply greater optimizations to lambdas that do not capture. I'll benchmark to be sure though.

[paulirwin]

After running some benchmarks, there is a slight performance and allocation hit for using lambdas over "AnonymousClasses" that implement a functional interface, when the lambda captures, and when this is done one-shot and not many times. There is basically no difference if they do not capture, or when done many times.

The one-shot performance impact is worse if the lambda captures a variable instead of a field, so the impact can be mitigated by storing it in a field (or wrapping it in your own equivalent of an "anonymous class") if this is a concern. It also allocates slightly more bytes.

I could go either way on this. The performance difference basically disappears at high enough usage, and I'd imagine Dynamic PGO would help this further. I don't know if 9 nanoseconds and 64 bytes for a one-shot call is enough to warrant harming the UX by making you create a custom class instead of passing a lambda to these APIs. I'm leaning towards still wanting this UX improvement.

Microsoft, if you're listening, please improve the performance and reduce the allocation of lambdas so that this is not a concern 😄

1 iteration:

| Method                                 | Iterations | Mean        | Error     | StdDev    | Gen0   | Allocated |
|--------------------------------------- |----------- |------------:|----------:|----------:|-------:|----------:|
| PredicateLambdaNoCapture               | 1          |   0.9096 ns | 0.0011 ns | 0.0009 ns |      - |         - |
| PredicateLambdaCaptureVariable         | 1          | 238.5972 ns | 0.2876 ns | 0.2550 ns | 0.0253 |     160 B |
| PredicateLambdaCaptureField            | 1          |   9.0264 ns | 0.0139 ns | 0.0123 ns | 0.0102 |      64 B |
| PredicateAnonymousClassNoCapture       | 1          |   0.0000 ns | 0.0000 ns | 0.0000 ns |      - |         - |
| PredicateAnonymousClassCaptureVariable | 1          | 229.3647 ns | 0.1341 ns | 0.1047 ns | 0.0153 |      96 B |
| PredicateAnonymousClassCaptureField    | 1          |   2.6220 ns | 0.0054 ns | 0.0050 ns | 0.0038 |      24 B |

100k iterations:

| Method                                 | Iterations | Mean     | Error    | StdDev   | Allocated |
|--------------------------------------- |----------- |---------:|---------:|---------:|----------:|
| PredicateLambdaNoCapture               | 100000     | 31.36 us | 0.030 us | 0.027 us |         - |
| PredicateLambdaCaptureVariable         | 100000     | 63.06 us | 0.046 us | 0.043 us |     160 B |
| PredicateLambdaCaptureField            | 100000     | 62.70 us | 0.034 us | 0.032 us |      64 B |
| PredicateAnonymousClassNoCapture       | 100000     | 31.33 us | 0.015 us | 0.012 us |         - |
| PredicateAnonymousClassCaptureVariable | 100000     | 63.03 us | 0.041 us | 0.036 us |      96 B |
| PredicateAnonymousClassCaptureField    | 100000     | 62.89 us | 0.348 us | 0.308 us |      24 B |

[Shazwazza]

Non static lambdas are just object instances and will allocate. NET does try to optimize this in many cases such as ConcurrentDictionary overloads like GetOrAdd allowing passing parameters in to the value builder to be used in the lambda as a parameters instead of a capture/closure. Anytime you can declare a lambda as static you absolutely should. This prevents allocations but means it cannot be a closure. See https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/static-anonymous-functions

…

On Sat, Dec 7, 2024, 9:10 a.m. Paul Irwin ***@***.***> wrote: After running some benchmarks, there is a slight performance and allocation hit for using lambdas over "AnonymousClasses" that implement a functional interface, when the lambda captures, and when this is done one-shot and not many times. There is basically no difference if they do not capture, or when done many times. The one-shot performance impact is worse if the lambda captures a variable instead of a field, so the impact can be mitigated by storing it in a field (or wrapping it in your own equivalent of an "anonymous class") if this is a concern. It also allocates slightly more bytes. I could go either way on this. The performance difference basically disappears at high enough usage, and I'd imagine Dynamic PGO would help this further. I don't know if 9 nanoseconds and 64 bytes for a one-shot call is enough to warrant harming the UX by making you create a custom class instead of passing a lambda to these APIs. I'm leaning towards still wanting this UX improvement. Microsoft, if you're listening, please improve the performance and reduce the allocation of lambdas so that this is not a concern 😄 1 iteration: | Method | Iterations | Mean | Error | StdDev | Gen0 | Allocated | |--------------------------------------- |----------- |------------:|----------:|----------:|-------:|----------:| | PredicateLambdaNoCapture | 1 | 0.9096 ns | 0.0011 ns | 0.0009 ns | - | - | | PredicateLambdaCaptureVariable | 1 | 238.5972 ns | 0.2876 ns | 0.2550 ns | 0.0253 | 160 B | | PredicateLambdaCaptureField | 1 | 9.0264 ns | 0.0139 ns | 0.0123 ns | 0.0102 | 64 B | | PredicateAnonymousClassNoCapture | 1 | 0.0000 ns | 0.0000 ns | 0.0000 ns | - | - | | PredicateAnonymousClassCaptureVariable | 1 | 229.3647 ns | 0.1341 ns | 0.1047 ns | 0.0153 | 96 B | | PredicateAnonymousClassCaptureField | 1 | 2.6220 ns | 0.0054 ns | 0.0050 ns | 0.0038 | 24 B | 100k iterations: | Method | Iterations | Mean | Error | StdDev | Allocated | |--------------------------------------- |----------- |---------:|---------:|---------:|----------:| | PredicateLambdaNoCapture | 100000 | 31.36 us | 0.030 us | 0.027 us | - | | PredicateLambdaCaptureVariable | 100000 | 63.06 us | 0.046 us | 0.043 us | 160 B | | PredicateLambdaCaptureField | 100000 | 62.70 us | 0.034 us | 0.032 us | 64 B | | PredicateAnonymousClassNoCapture | 100000 | 31.33 us | 0.015 us | 0.012 us | - | | PredicateAnonymousClassCaptureVariable | 100000 | 63.03 us | 0.041 us | 0.036 us | 96 B | | PredicateAnonymousClassCaptureField | 100000 | 62.89 us | 0.348 us | 0.308 us | 24 B | — Reply to this email directly, view it on GitHub <#1062 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANJOXLZSPM6SPITEDTKYWD2EMMX3AVCNFSM6AAAAABTFCDAWWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMRVGIZDQNRWHE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[paulirwin]

@Shazwazza Thanks, I made the no-capture lambda case static and it was the same result, so I'm guessing .NET was already optimizing it that way. The others I can't make static because they capture, like you said. It would be nice if .NET somehow could optimize a non-static lambda to not be more costly than an object that implements a functional interface...

Inspired by your comment about overloads, perhaps we could add overloads that take a delegate but then store it in a lucenenet-specific implementation of the interface that just calls the delegate, so that the underlying stored reference is the functional interface type instead of a delegate. This would probably be the best of both worlds: allow for ease of use in the API by passing a lambda if you're not concerned about a few ns/op or some extra bytes allocated, but allow for passing an object that implements the interface for more performance-critical scenarios. I'll experiment with this approach and see how it feels.

[paulirwin]

After some thought, given the performance concern, I'm going to move this to the Future milestone. I still think this will warrant consideration in the future, but I think it should be once we have good benchmarking infrastructure in place to ensure there are no performance regressions. In the meantime, this would just be a convenience and users can easily work around it by creating their own wrapper types, such as:

public class DelegateReaderDisposedListener : IReaderDisposedListener
{
      private readonly Action<IndexReader> action;

      public DelegateReaderDisposedListener(Action<IndexReader> action)
      {
          this.action = action;
      }

      public void OnDispose(IndexReader reader)
      {
          action(reader);
      }
}