You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucenenet.apache.org by "NightOwl888 (via GitHub)" <gi...@apache.org> on 2026/09/09 04:00:22 UTC
Re: [PR] SWEEP: Removal of ICharSequence from most places, #1337 [lucenenet]
NightOwl888 commented on code in PR #1387:
URL: https://github.com/apache/lucenenet/pull/1387#discussion_r3960118493
##########
src/Lucene.Net.Analysis.Common/Analysis/Util/CharArrayMap.cs:
##########
@@ -299,28 +270,6 @@ public virtual void Add(char[] text, TValue value)
Set(text, value);
}
- /// <summary>
- /// Adds the <paramref name="value"/> for the passed in <paramref name="text"/>.
- /// </summary>
- /// <param name="text">The string-able type to be added/updated in the dictionary.</param>
- /// <param name="value">The corresponding value for the given <paramref name="text"/>.</param>
- /// <exception cref="ArgumentNullException">
- /// <paramref name="text"/> is <c>null</c>.
- /// <para/>
- /// -or-
- /// <para/>
- /// The <paramref name="text"/>'s <see cref="ICharSequence.HasValue"/> property returns <c>false</c>.
- /// </exception>
- /// <exception cref="ArgumentException">An element with <paramref name="text"/> already exists in the dictionary.</exception>
- public virtual void Add(ICharSequence text, TValue value)
Review Comment:
In this case, we can and should convert this to a `ReadOnlySpan<char>` overload, since it is missing one, although, it has all of the support to implement one. It internally converts the span to an array, but since it is immediately adding it to a collection, this isn't as bad as allocating a temporary array that immediately is left for garbage collection.
##########
src/Lucene.Net.Facet/Taxonomy/WriterCache/CharBlockArray.cs:
##########
@@ -176,160 +177,12 @@ public virtual CharBlockArray Append(ICharSequence? value, int startIndex, int l
if (startIndex > value.Length - length)
throw new ArgumentOutOfRangeException(nameof(startIndex), $"Index and length must refer to a location within the string. For example {nameof(startIndex)} + {nameof(length)} <= {nameof(Length)}.");
-
int end = startIndex + length;
for (int i = startIndex; i < end; i++)
Review Comment:
This slow implementation made sense for `ICharSequence`, but it doesn't here.
That said, it would be better to make this type implicitly convertible to `ReadOnlySpan<char>` and change the `ReadOnlySpan<char>` overload to be safe for inserting the same memory location. I recently did that in `MutableTextBuffer` - here are the links.
https://github.com/NightOwl888/J2N/blob/a9c6092b14bbbbca6c0f842dcee7613582fcc4f2/src/J2N/Text/MutableTextBuffer.TextSources.cs#L206-L228
https://github.com/NightOwl888/J2N/blob/a9c6092b14bbbbca6c0f842dcee7613582fcc4f2/src/J2N/Text/MutableTextBuffer.TextSources.cs#L1039-L1076
This would transform it into a bulk operation that uses SIMD optimizations rather than a slow stream of adding one character at a time.
Note that we can also remove the `startIndex`/`length` overload if we add `AsSpan()` extension methods so we can slice this type.
##########
src/Lucene.Net.Tests.Facet/Taxonomy/WriterCache/TestCharBlockArray.cs:
##########
@@ -221,7 +221,7 @@ public virtual void TestAppendableInterface()
try
{
- t.Append((char[])t2.ToString().ToCharArray(), 1, 0 - 1); // LUCENENET: Corrected 3rd parameter
+ t.Append(t2.ToString().ToCharArray().AsSpan(1, 0 - 1)); // LUCENENET: Corrected parameter
Review Comment:
Looks like there are several more cases where `.ToCharArray().AsSpan()` are chained together in this class, which seems odd.
Note this also applies to the TestCharTermAttributeImpl class where this was poached from.
##########
src/Lucene.Net/Analysis/Token.cs:
##########
@@ -448,47 +442,11 @@ private void ClearNoTermBuffer()
/// <summary>
/// Shorthand for calling <see cref="Clear"/>,
- /// <see cref="ICharTermAttribute.CopyBuffer(char[], int, int)"/>,
- /// <see cref="SetOffset"/>,
- /// <see cref="Type"/> (set) </summary>
- /// <returns> this <see cref="Token"/> instance </returns>
- public virtual Token Reinit(char[] newTermBuffer, int newTermOffset, int newTermLength, int newStartOffset, int newEndOffset, string newType)
- {
- CheckOffsets(newStartOffset, newEndOffset);
- ClearNoTermBuffer();
- CopyBuffer(newTermBuffer, newTermOffset, newTermLength);
- payload = null;
- positionIncrement = 1;
- startOffset = newStartOffset;
- endOffset = newEndOffset;
- type = newType;
- return this;
- }
-
- /// <summary>
- /// Shorthand for calling <see cref="Clear"/>,
- /// <see cref="ICharTermAttribute.CopyBuffer(char[], int, int)"/>,
- /// <see cref="SetOffset"/>,
- /// <see cref="Type"/> (set) on <see cref="TypeAttribute.DEFAULT_TYPE"/> </summary>
- /// <returns> this <see cref="Token"/> instance </returns>
- public virtual Token Reinit(char[] newTermBuffer, int newTermOffset, int newTermLength, int newStartOffset, int newEndOffset)
- {
- CheckOffsets(newStartOffset, newEndOffset);
- ClearNoTermBuffer();
Review Comment:
This is a bit odd. These methods existed upstream as they were here. They didn't use `CharSequence`. And now that these 2 overloads have been removed, there are no callers to the `ClearNoTermBuffer()` method. What is odd is that there are no callers to any of these methods. So, it isn't clear how they were intended to be used or whether excluding these overloads that have different clear behavior would be breaking behavior for any callers.
Maybe the safest thing to do would be to revert this class in this PR, add tests for it in a separate PR, and then decide whether we can make this sort of change.
##########
src/Lucene.Net.Analysis.Common/Analysis/Util/CharArrayMap.cs:
##########
@@ -440,7 +389,7 @@ private void CopyTo(KeyValuePair<string, TValue>[] array, int arrayIndex)
/// <exception cref="ArgumentOutOfRangeException"><paramref name="index"/> is less than zero.</exception>
/// <exception cref="ArgumentException">The number of elements in the source is greater
/// than the available space from <paramref name="index"/> to the end of the destination array.</exception>
- internal void CopyTo(KeyValuePair<ICharSequence, TValue>[] array, int index) // internal for testing
+ internal void CopyTo(KeyValuePair<ReadOnlyMemory<char>, TValue>[] array, int index) // internal for testing
Review Comment:
`ReadOnlyMemory<char>` doesn't own the pointer that keeps the memory in scope. Cloning the `KeyValuePair` is a bit dangerous because the caller must be aware when the source of the memory backing it goes out of scope.
Note in ICU4N I created a `CharSequence` type to be used in collections that owns both the `ReadOnlyMemory<char>` instance and a reference to the memory to keep it in scope.
However, we should analyze this further.
1. We should remove all of the code that accepts arbitrary objects that are converted using formatting (generic overloads and numeric overloads).
2. Now that we have alternate lookup implemented in all target frameworks, it would probably be better to back this by a `Dictionary<string, TValue>`. Then the lookup can either be `string` or `ReadOnlySpan<char>` (which covers `char[]` and `TextBuilder`).
Anyway, that doesn't have to be done in this PR, but we should probably be less eager to use `ReadOnlyMemory<char>` due to its inability to manage its own scope.
##########
src/Lucene.Net.Analysis.Common/Analysis/Compound/CompoundWordTokenFilterBase.cs:
##########
@@ -174,7 +175,7 @@ protected class CompoundToken
/// Construct the compound token based on a slice of the current <see cref="CompoundWordTokenFilterBase.m_termAtt"/>. </summary>
public CompoundToken(CompoundWordTokenFilterBase compoundWordTokenFilterBase, int offset, int length)
{
- this.txt = compoundWordTokenFilterBase.m_termAtt.Subsequence(offset, length); // LUCENENET: Corrected 2nd Subsequence parameter
+ this.txt = compoundWordTokenFilterBase.m_termAtt.AsMemory(offset, length); // LUCENENET: Corrected 2nd Subsequence parameter
Review Comment:
This is a fundamentally different operation than upstream. There are 2 different problems here:
1. `Subsequence()` works similarly to `Substring()` - that is, it creates a new chunk of memory that is independent of the original and copies the chars from the original memory location into it.
2. Storing `ReadOnlyMemory<char>` here is dangerous without also storing a reference to the class that is behind it. In ICU4N, we use [a helper method](https://github.com/NightOwl888/ICU4N/blob/main/src/ICU4N/Support/MemoryExtensions.cs#L157-L181) and `object` field to ensure the backing type is kept in scope.
So, the new operation adds a dependency on the stability of the backing array of `m_termAtt`. And since it is protected, there are no guarantees there.
Usually, when I encounter a type like this, there are a couple of questions to ask:
1. Is this type suitable for converting into a ref struct?
2. How long is the memory kept in scope until it is garbage collected?
Keep in mind, there is nothing inherently bad about allocating heap. The problems occur when that heap is allocated, used only for a very short time, and then left for the GC to clean up. In this case, after the memory is allocated, it is added to a queue for later processing. So, more analysis is needed to determine how quickly that queue cycles its memory.
One thing I noticed that could be impactful is the fact that upstream the `queue` was a linked list and we have changed it to be a `Queue<T>`. When resetting, both of them clear out all of their memory for garbage collection and `Queue<T>` potentially reuses its backing array, while `LinkedList<T>` would not allocate a backing array at all. So, it would be interesting to benchmark to work out if either of them would be a clear winner.
There are a couple of ways I can think of that would potentially improve reuse, but both come with some extra complexity.
1. Have `CompoundToken` implement `IDisposable` and back it with chunks of memory from the shared array pool.
2. Allocate a single chunk of heap in the `CompoundWordTokenFilterBase` and have each `CompoundToken` instance use a slice of it until it is full, after which new chunks are allocated for each token.
Combining both of these might be a 3rd option.
Failing any of those, the minimal safe way to remove `ICharSequence` here is to convert the backing field into a `string` that is allocated in the constructor. It is essentially the same thing we have now, but without the baggage of `CharArrayCharSequence`.
##########
src/Lucene.Net.Analysis.Common/Analysis/Util/CharArrayMap.cs:
##########
@@ -930,11 +740,9 @@ public virtual bool Put<T>(T text, TValue value, [MaybeNullWhen(returnValue: tru
// than casting using *as* and then checking for null.
// http://stackoverflow.com/q/1583050/181087
if (text is string str)
- return PutImpl(str, value);
+ return PutImpl(str.AsSpan(), value);
if (text is char[] charArray)
- return PutImpl(charArray, value);
- if (text is ICharSequence cs)
- return PutImpl(cs, value);
+ return PutImpl(charArray.AsSpan(), value);
Review Comment:
Was copying the array intentional? The original implementation was set up to put the same instance as the one passed in, not a copy. I am not saying that was right, but this is a difference in behavior.
##########
src/Lucene.Net.Tests.Facet/Taxonomy/WriterCache/TestCharBlockArray.cs:
##########
@@ -194,7 +194,7 @@ public virtual void TestAppendableInterface()
try
{
- t.Append((string)t2.ToString(), 1, 0 - 1); // LUCENENET: Corrected 3rd parameter
+ t.Append(t2.ToString().AsSpan(1, 0 - 1)); // LUCENENET: Corrected parameter
Review Comment:
This is an odd choice. Why not just call `t2.AsSpan(1, 0 - 1)`?
Note this also applies to the `TestCharTermAttributeImpl` class where this was poached from.
##########
src/Lucene.Net.Tests.Facet/Taxonomy/WriterCache/TestCharBlockArray.cs:
##########
@@ -210,7 +210,7 @@ public virtual void TestAppendableInterface()
// LUCENENET specific - test char[] overloads
try
{
- t.Append((char[])t2.ToString().ToCharArray(), 1, 5 - 1); // LUCENENET: Corrected 3rd parameter
+ t.Append(t2.ToString().ToCharArray().AsSpan(1, 5 - 1)); // LUCENENET: Corrected parameter
Review Comment:
This is an odd choice. Why not just call `t2.AsSpan(1, 0 - 1)`?
Note this also applies to the `TestCharTermAttributeImpl` class where this was poached from.
##########
src/Lucene.Net/Analysis/TokenAttributes/CharTermAttribute.cs:
##########
@@ -94,146 +94,12 @@ public interface ICharTermAttribute : IAttribute, ICharSequence, IAppendable,
/// <seealso cref="CharTermAttributeExtensions.SetEmpty{T}(T)"/>
void Clear();
- /// <summary>
- /// Appends the contents of the <see cref="ICharSequence"/> to this character sequence.
- /// <para/>
- /// The characters of the <see cref="ICharSequence"/> argument are appended, in order, increasing the length of
- /// this sequence by the length of the argument. If <paramref name="value"/> is <c>null</c>, this method is a no-op.
- /// <para/>
- /// IMPORTANT: This method uses .NET semantics. In Lucene, a <c>null</c> <paramref name="value"/> would append the
- /// string <c>"null"</c> to the instance, but in Lucene.NET a <c>null</c> value will be ignored.
- /// </summary>
- new ICharTermAttribute Append(ICharSequence value);
-
- /// <summary>
- /// Appends the a string representation of the specified <see cref="ICharSequence"/> to this instance.
- /// <para>
- /// The characters of the <see cref="ICharSequence"/> argument are appended, in order, increasing the length of
- /// this sequence by the length of <paramref name="count"/>. If <paramref name="value"/> is <c>null</c>
- /// and <paramref name="startIndex"/> and <paramref name="count"/> are not 0, an
- /// <see cref="ArgumentNullException"/> is thrown.
- /// </para>
- /// </summary>
- /// <param name="value">The sequence of characters to append.</param>
- /// <param name="startIndex">The start index of the <see cref="ICharSequence"/> to begin copying characters.</param>
- /// <param name="count">The number of characters to append.</param>
- /// <exception cref="ArgumentNullException"><paramref name="value"/> is <c>null</c>, and
- /// <paramref name="startIndex"/> and <paramref name="count"/> are not zero.</exception>
- /// <exception cref="ArgumentOutOfRangeException">
- /// <paramref name="count"/> is less than zero.
- /// <para/>
- /// -or-
- /// <para/>
- /// <paramref name="startIndex"/> is less than zero.
- /// <para/>
- /// -or-
- /// <para/>
- /// <paramref name="startIndex"/> + <paramref name="count"/> is greater than the length of <paramref name="value"/>.
- /// </exception>
- new ICharTermAttribute Append(ICharSequence value, int startIndex, int count); // LUCENENET: changed to startIndex/length to match .NET
-
/// <summary>
/// Appends the supplied <see cref="char"/> to this character sequence.
/// </summary>
/// <param name="value">The <see cref="char"/> to append.</param>
new ICharTermAttribute Append(char value);
- /// <summary>
- /// Appends the contents of the <see cref="T:char[]"/> array to this character sequence.
- /// <para/>
- /// The characters of the <see cref="T:char[]"/> argument are appended, in order, increasing the length of
- /// this sequence by the length of the <paramref name="value"/>.
- /// If <paramref name="value"/> is <c>null</c>, this method is a no-op.
- /// <para/>
- /// This method uses .NET semantics. In Lucene, a <c>null</c> <paramref name="value"/> would append the
- /// string <c>"null"</c> to the instance, but in Lucene.NET a <c>null</c> value will be safely ignored.
- /// </summary>
- /// <param name="value">The <see cref="T:char[]"/> array to append.</param>
- /// <remarks>
- /// LUCENENET specific method, added to simulate using the CharBuffer class in Java.
- /// </remarks>
- new ICharTermAttribute Append(char[] value);
-
- /// <summary>
- /// Appends the string representation of the <see cref="T:char[]"/> array to this instance.
- /// <para>
- /// The characters of the <see cref="T:char[]"/> argument are appended, in order, increasing the length of
- /// this sequence by the length of the <paramref name="value"/>. If <paramref name="value"/> is <c>null</c>
- /// and <paramref name="startIndex"/> and <paramref name="count"/> are not 0, an
- /// <see cref="ArgumentNullException"/> is thrown.
- /// </para>
- /// </summary>
- /// <param name="value">The sequence of characters to append.</param>
- /// <param name="startIndex">The start index of the <see cref="T:char[]"/> to begin copying characters.</param>
- /// <param name="count">The number of characters to append.</param>
- /// <exception cref="ArgumentNullException"><paramref name="value"/> is <c>null</c>, and
- /// <paramref name="startIndex"/> and <paramref name="count"/> are not zero.</exception>
- /// <exception cref="ArgumentOutOfRangeException">
- /// <paramref name="count"/> is less than zero.
- /// <para/>
- /// -or-
- /// <para/>
- /// <paramref name="startIndex"/> is less than zero.
- /// <para/>
- /// -or-
- /// <para/>
- /// <paramref name="startIndex"/> + <paramref name="count"/> is greater than the length of <paramref name="value"/>.
- /// </exception>
- /// <remarks>
- /// LUCENENET specific method, added to simulate using the CharBuffer class in Java. Note that
- /// the <see cref="CopyBuffer(char[], int, int)"/> method provides similar functionality.
- /// </remarks>
- new ICharTermAttribute Append(char[] value, int startIndex, int count); // LUCENENET: changed to startIndex/length to match .NET
-
- /// <summary>
- /// Appends the specified <see cref="string"/> to this character sequence.
- /// <para>
- /// The characters of the <see cref="string"/> argument are appended, in order, increasing the length of
- /// this sequence by the length of the argument. If argument is <c>null</c>, this method is a no-op.
- /// <para/>
- /// This method uses .NET semantics. In Lucene, a <c>null</c> <paramref name="value"/> would append the
- /// string <c>"null"</c> to the instance, but in Lucene.NET a <c>null</c> value will be safely ignored.
- /// </para>
- /// </summary>
- /// <param name="value">The sequence of characters to append.</param>
- /// <remarks>
- /// LUCENENET specific method, added because the .NET <see cref="string"/> data type
- /// doesn't implement <see cref="ICharSequence"/>.
- /// </remarks>
- new ICharTermAttribute Append(string value);
Review Comment:
See my message in the `CharTermAttributeImpl.cs` file. We should keep supporting the overloads for specific data types because converting to `ReadOnlySpan<char>` is still less efficient than having a `string`, `char[]`, overload, etc.
##########
src/Lucene.Net/Analysis/TokenAttributes/CharTermAttributeImpl.cs:
##########
@@ -166,101 +164,14 @@ public char this[int index]
}
}
- public ICharSequence Subsequence(int startIndex, int length)
- {
- // From Apache Harmony String class
- if (termBuffer is null || (startIndex == 0 && length == termBuffer.Length))
- {
- return new CharArrayCharSequence(termBuffer);
- }
- if (startIndex < 0)
- throw new ArgumentOutOfRangeException(nameof(startIndex), $"{nameof(startIndex)} must not be negative.");
- if (length < 0)
- throw new ArgumentOutOfRangeException(nameof(length), $"{nameof(length)} must not be negative.");
- if (startIndex > Length - length) // Checks for int overflow
- throw new ArgumentOutOfRangeException(nameof(length), $"Index and length must refer to a location within the string. For example {nameof(startIndex)} + {nameof(length)} <= {nameof(Length)}.");
-
- char[] result = new char[length];
- Arrays.Copy(termBuffer, startIndex, result, 0, length);
- return new CharArrayCharSequence(result);
- }
-
// *** Appendable interface ***
- public CharTermAttribute Append(string value, int startIndex, int charCount)
- {
- // LUCENENET: Changed semantics to be the same as the StringBuilder in .NET
- if (startIndex < 0)
- throw new ArgumentOutOfRangeException(nameof(startIndex), $"{nameof(startIndex)} must not be negative.");
- if (charCount < 0)
- throw new ArgumentOutOfRangeException(nameof(charCount), $"{nameof(charCount)} must not be negative.");
-
- if (value is null)
- {
- if (startIndex == 0 && charCount == 0)
- return this;
- throw new ArgumentNullException(nameof(value));
- }
- if (charCount == 0)
- return this;
- if (startIndex > value.Length - charCount)
- throw new ArgumentOutOfRangeException(nameof(startIndex), $"Index and length must refer to a location within the string. For example {nameof(startIndex)} + {nameof(charCount)} <= {nameof(Length)}.");
-
- value.CopyTo(startIndex, InternalResizeBuffer(termLength + charCount), termLength, charCount);
- Length += charCount;
-
- return this;
- }
-
public CharTermAttribute Append(char value)
{
ResizeBuffer(termLength + 1)[termLength++] = value;
return this;
}
- public CharTermAttribute Append(char[] value)
- {
- if (value is null)
- //return AppendNull();
- return this; // No-op
-
- int len = value.Length;
- value.CopyTo(InternalResizeBuffer(termLength + len), termLength);
- Length += len;
-
- return this;
- }
-
- public CharTermAttribute Append(char[] value, int startIndex, int charCount)
- {
- // LUCENENET: Changed semantics to be the same as the StringBuilder in .NET
- if (startIndex < 0)
- throw new ArgumentOutOfRangeException(nameof(startIndex), $"{nameof(startIndex)} must not be negative.");
- if (charCount < 0)
- throw new ArgumentOutOfRangeException(nameof(charCount), $"{nameof(charCount)} must not be negative.");
-
- if (value is null)
- {
- if (startIndex == 0 && charCount == 0)
- return this;
- throw new ArgumentNullException(nameof(value));
- }
- if (charCount == 0)
- return this;
- if (startIndex > value.Length - charCount)
- throw new ArgumentOutOfRangeException(nameof(startIndex), $"Index and length must refer to a location within the string. For example {nameof(startIndex)} + {nameof(charCount)} <= {nameof(Length)}.");
-
- Arrays.Copy(value, startIndex, InternalResizeBuffer(termLength + charCount), termLength, charCount);
- Length += charCount;
-
- return this;
- }
-
- public CharTermAttribute Append(string value)
Review Comment:
This overload exists upstream. Despite the fact that `string` can be implicitly converted to `ReadOnlySpan<char>` it is still more efficient for it to have its own overload than to rely only on `ReadOnlySpan<char>` for this common case. Given that this is a high use user API, we should provide the direct overloads to optimize it.
That being said, the upstream implementation was also better than it was here. We should effectively copy the `StringBuilder` overload implementation, since using `string.CopyTo()` is the most efficient way to transfer the characters into the local buffer.
It should also be made nullable to indicate that it won't throw if the value is `null`, instead it will be a no-op.
It probably also makes sense to have the `char[]` overloads even though they don't exist upstream for the same reason.
The plan is to remove the `IAppendable` interface, not the 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: dev-unsubscribe@lucenenet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org