Use concrete collection types for private fields and variables

[paulirwin]

Is there an existing issue for this?

• I have searched the existing issues

Task description

I noticed when reviewing #1200 that the existing code (example) sometimes uses interfaces like IDictionary<TKey, TValue> for the type of private fields and variables, instead of the concrete type. While this matches upstream and helps ensure you're coding to the contract rather than the implementation, this might be accidentally harming performance.

By typing the field/variable to the interface, we lose out on some optimizations like how OrderedDictionary uses a struct enumerator to avoid an allocation/boxing. Also, the JIT can more aggressively inline, eliminate bounds checks, vectorize, etc. when the type is concrete.

Microsoft has the CA1859 analyzer specifically to flag this, and this can be used to make finding issues and refactoring easier.

Note that we should only do this for private fields (and potentially other private members, like return types of private methods) as well as variables, so that we are not changing the public contracts. A comment should be added mentioning CA1859, i.e. // LUCENENET: CA1859 - Use concrete types when possible for improved performance

While there is likely benefit beyond collections, let's start small and not "boil the ocean" here. Collections are where there is the biggest benefit due to things like stack allocated enumerators and possible vectorization.

I think this is worth evaluating as a deviation from exactly matching upstream Lucene code. cc @NightOwl888 for thoughts/feedback.

[NightOwl888]

I agree. I think the analyzer is smart enough not to suggest this fix if we are passing an interface through the constructor, but we would get a compiler error if those were "fixed" and that were not allowed.

For the most part, this won't have an effect until J2N 3.0 when we are able to address the GetEnumerator() method return types, but having this in place before that release is fine.

[Jivi-this-side]

Hello @paulirwin sir!
I went through the issue details and :

lucenenet/src/Lucene.Net/Util/DoubleBarrelLRUCache.cs

Line 46 in 5b48701

private readonly IDictionary<TKey, TValue> cache1;

If I’m understanding correctly, this task involves updating private fields (and possibly local variables) that currently use interface types like IDictionary<TKey, TValue> or IList<T> to instead use their concrete implementations (e.g., Dictionary<TKey, TValue>, List, etc.) — while keeping public contracts unchanged.

// Before
private readonly IDictionary<string, object> cache1;

// After
private readonly Dictionary<string, object> cache2; // LUCENENET: CA1859 - Use concrete types when possible for improved performance

If this understanding is correct, I’d be happy to work on this improvement. Please let me know if you’d like me to start with the collections in this file or across other similar areas as well.

Thank you! 🙌

[NightOwl888]

@Jivi-this-side

Thanks for volunteering. I have assigned this issue to you. Yes, your understanding of how to address the issue is correct.

As I previously mentioned, there are some cases where we are accepting a collection interface through the constructor which is the best way we can support both BCL and J2N collection types. In those cases we won't be able to replace the private fields. However, in most cases these collection are instantiated internally, so replacing the field type is fine. I could be wrong, but I am pretty sure that the CA1859 rule is smart enough not to suggest this when the type is set from the constructor.

Ideally, this would be a subtle change that doesn't deviate too far from the upstream code. If you find that replacing one tends to involve more plumbing than just changing the field type, it might be a good idea to check with us to determine if there is a better approach.

Also, note that we have extension methods on the collection interface types. In most cases, these are passing the call through to the underlying collection. However, there may also be cases where the collection API doesn't exist in .NET Framework or .NET Standard. If you run into that issue, let us know, as that may require more analyisis.

This task also should ideally not change any collection types. We mostly use collections from J2N. This is intentional because of various behaviors that they have (such as structural equality) that Lucene or its tests depend upon.