You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by "paulirwin (via GitHub)" <gi...@apache.org> on 2025/10/04 20:55:00 UTC
[I] Use concrete collection types for private fields and variables [lucenenet]
paulirwin opened a new issue, #1201:
URL: https://github.com/apache/lucenenet/issues/1201
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Task description
I noticed when reviewing #1200 that the [existing code (example)](https://github.com/apache/lucenenet/pull/1200/files#diff-092b352b0e745a7f63744637e0e854c22e44fe2f12624b8f95640ac0327de86cR80) 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](https://github.com/NightOwl888/J2N/blob/main/src/J2N/Collections/Generic/OrderedDictionary.cs#L1373). Also, the JIT can more aggressively inline, eliminate bounds checks, vectorize, etc. when the type is concrete.
Microsoft has the [CA1859](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Use concrete collection types for private fields and variables [lucenenet]
Posted by "Jivi-this-side (via GitHub)" <gi...@apache.org>.
Jivi-this-side commented on issue #1201:
URL: https://github.com/apache/lucenenet/issues/1201#issuecomment-3392919260
Hello @paulirwin sir!
I went through the issue details and :
https://github.com/apache/lucenenet/blob/5b487018599ae7ab064f628fbdc9f1f5df6dc6e4/src/Lucene.Net/Util/DoubleBarrelLRUCache.cs#L46
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<T>, 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! 🙌
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Use concrete collection types for private fields and variables [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on issue #1201:
URL: https://github.com/apache/lucenenet/issues/1201#issuecomment-4269943346
@Jivi-this-side Checking in, how is this going? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Use concrete collection types for private fields and variables [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on issue #1201:
URL: https://github.com/apache/lucenenet/issues/1201#issuecomment-3698188651
Hey @Jivi-this-side - how is this going?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Use concrete collection types for private fields and variables [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on issue #1201:
URL: https://github.com/apache/lucenenet/issues/1201#issuecomment-4560524721
I'm going to take this one over and finish it up, since it's becoming stale. Thanks again for looking into it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Use concrete collection types for private fields and variables [lucenenet]
Posted by "paulirwin (via GitHub)" <gi...@apache.org>.
paulirwin commented on issue #1201:
URL: https://github.com/apache/lucenenet/issues/1201#issuecomment-4564726165
I started working on this last night to just help pick off some of the low-hanging beta 18 items, but realized that by definition this is not a breaking change, and thus doesn't need to be done for beta 18. I am not sure why I put it in this milestone originally. Moving to 4.8.0 final milestone.
If anyone wants to pick this up in the future, I'm unassigning it from myself. I started some work here to see how it is done and how the `.editorconfig` rule enables the warning, that can be used as a starting point: https://github.com/paulirwin/lucene.net/tree/issue/1201
But again, this is low priority, and doesn't need to be done for beta 18. There are many other higher priority beta 18 items to pick above this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Use concrete collection types for private fields and variables [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on issue #1201:
URL: https://github.com/apache/lucenenet/issues/1201#issuecomment-3392934574
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Use concrete collection types for private fields and variables [lucenenet]
Posted by "NightOwl888 (via GitHub)" <gi...@apache.org>.
NightOwl888 commented on issue #1201:
URL: https://github.com/apache/lucenenet/issues/1201#issuecomment-3369801477
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org