NightOwl888 commented on PR #1108:
URL: https://github.com/apache/lucenenet/pull/1108#issuecomment-2604677372
1. True, which is why we tried avoiding that by cascading the call to a
wraped BCL implementation previously. Unfortunately, the tradeoff is that prior
to .NET Core 3.x there was no support for deletion while iterating forward
which Lucene uses in a few places. So, we either have to conditionally compile
or make a normalized implementation in J2N. IMO, it is better to have a
normalized implementation so we can push as many conditional compilation
statements into J2N as possible until such time we don't need the J2N
implementation any longer (in this case, when we drop support for everything
below .NET Core). But, I don't think we should use `JCG.Dictionary<TKey,
TValue>` unless it is required.
2. All I am saying is that the next release of Lucene.NET will not have a
reference to J2N 2.1, so this comparison is not completely accurate as to what
will be released and does not account for work that already exists on the
`main` branch in J2N.
In the benchmark the compiler `foreach` calls `SCG.Dictionary<TKey,
TValue>.Values.GetEnumerator()`, which returns type `SCG.Dictionary<TKey,
TValue>.ValueCollection.Enumerator`, a struct. The J2N implementation also
returns a struct, but casts it to an `IEnumerator<TValue>`, which will box. To
do a direct comparison, the dictionary should be using the interface
`IDictionary<string, string> values = new Dictionary<string, string>();` so the
box happens there, also. In J2N 3.0, this boxing will go away so the concrete
types will return a struct which will match the upstream performance better.
Also, J2N 2.1 had some boxing in some of the guard clauses, which was
[recently fixed](https://github.com/NightOwl888/J2N/pull/127) as well as using
the [unsigned right shift operator instead of a
method](https://github.com/NightOwl888/J2N/pull/116). These will have a
positive impact on the performance of much of the J2N library, including the
collections.
3. Good point on the difference in hardware. That could be causing the
warnings I am seeing. Of course, that makes me cautious about seeing results
from others that don't have at least 100ms of work in the benchmark.
#### .NET Framework
There is one other difference that could explain what you are seeing on .NET
Framework. We patched the implementation to nullify the data buckets only for
reference types, whereas .NET Framework did it for both value and reference
types. There is a method called
`RuntimeHelpers.IsReferenceOrContainsReferences<T>()` that is used to determine
whether there are any reference types to nullify, but it doesn't exist on .NET
Framework so we use Reflection to analyze the type and cache the value the
first time a type is used. More iterations would definitely change the result
because that Reflection overhead only happens on the first call. Here is the
implementation:
https://github.com/NightOwl888/J2N/blob/8845a696e2cd3e2ca0752823c779c72aa7f0e5a1/src/J2N/Runtime/CompilerServices/RuntimeHelper.cs#L31-L84
So, 2 ways we could deal with this:
- Review the above implementation to see if there are any performance
enhancements that could be made
- Change it to always clean up references the collection buckets as was the
case in .NET Framework but keep the enhancement where
`RuntimeHelpers.IsReferenceOrContainsReferences<T>()` exists (which would
require conditional compilation in the J2N collections)
#### .NET 9 Implementation
Actually, it is possible that the difference between `net8.0` and `net9.0`
is due to a more efficient `.ToArray()` implementation and this might have
little or nothing to do with enhancements to the `Dictionary<TKey, TValue>`
implementation.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
