[StringUtils::indexOfAnyBut] redesign due to inconsistent/faulty behavior regarding UTF-16 surrogates

[IBue]

depends/stacked on #1326 (tests only)

Both signatures of StringUtils::indexOfAnyBut currently behave
inconsistently in matching UTF-16 supplementary characters and single
UTF-16 surrogate characters (i.e. paired and unpaired surrogates), since
they differ unnecessarily in their algorithmic implementations, use
their own incomplete and faulty interpretation of UTF-16 and don't take
full advantage of the standard library.

The example cases below show that they may yield contradictory results
or correct results for the wrong reasons.

This proposal gives a unified algorithmic implementation of both
signatures that

• a) is much easier to grasp due to a clear mathematical set approach and
  safe iteration and doesn't become entangled in index arithmetic;
  stresses the set semantics of the 2nd argument
• b) fully relies on the standard library for defined UTF-16
  handling/interpretation;
  paired surrogates are merged into one codepoint, unpaired surrogates
  are left as they are
• c) scales much better with input sizes and result index position
• d) can benefit from current and future improvements in the standard
  library and JVM
  (streams implementation, parallelization, JIT optimization, JEP 218, ???…)

The algorithm boils down to:
find index i of first char in cs such that
(cs.codePointAt(i) ∈ {x ∈ codepoints(cs) ∣ x ∉ codepoints(searchChars) })
(cs.codePointAt(i) ∉ { x ∈ codepoints(searchChars) })

Examples:

<H>: high-surrogate character
<L>: low-surrogate character
(<H><L>): valid supplementary character
signature 1: StringUtils::indexOfAnyBut(final CharSequence seq, final CharSequence searchChars)
signature 2: StringUtils::indexOfAnyBut(final CharSequence cs, final char... searchChars)

Case 1: matching of unpaired high-surrogate

	seq/cs	searchChars	expected/new	sig.1	sig.2
1.1	<H>aaaa	<H>abcd	!found	!found	!found
				sig.1: 'a' is somewhere in searchChars	sig.2: 'a' happens to follow <H> in searchChars
1.2	<H>baaa	<H>abcd	!found	!found	0
				sig.1: 'b' is somewhere in searchChars
1.3	<H>aaaa	(<H><L>)abcd	0	!found	0
				sig.1: 'a' is somewhere in searchChars
1.4	aaaa<H>	(<H><L>)abcd	4	!found	!found

Case 2: matching of unpaired low-surrogate

	seq/cs	searchChars	expected/new	sig.1	sig.2
2.1	<L>aaaa	(<H><L>)abcd	0	!found	!found
					sig.1+2 don't interpret suppl. character
2.2	aaaa<L>	(<H><L>)abcd	4	!found	!found
					sig.1+2 don't interpret suppl. character

Case 3: matching of supplementary character

	seq/cs	searchChars	expected/new	sig.1	sig.2
3.1	(<H><L>)aaaa	<L>ab<H>cd	0	!found	0
				sig.1: <L> is somewhere in searchChars
3.2	(<H><L>)aaaa	abcd	0	1	0
				sig.1 always points to low-surrogate of (fully) unmatched suppl. character
3.3	(<H><L>)aaaa	abcd<H>	0	0	1
3.4	(<H><L>)aaaa	abcd<L>	0	!found	0
				sig.1: <H> skipped by algorithm

[garydgregory]

Hello @IBue

Please rebate on git master and see my comments. It's hard to read the changes since some changes make the PR noisier than it has to be, like unnecessary parameter name changes. Also the PR contains duplicate work from your other PR.

TY!

[garydgregory]

Don't rename parameters in methods, it makes the PR noisy and makes it harder to review.

[garydgregory]

These changes duplicate your changes in your other PR. Rebase on git master and don't make these changes here.

[garydgregory]

Not sure why you inlined these calls. Keep the PR to only the required changes to fix a bug.

[garydgregory]

What does comments like >=10, 11, ... mean here? Can you be more explicit? Are these magic numbers?

[IBue]

I have rebased onto master and have followed your comments.

[garydgregory]

This one needs a second pair of eyes... @ppkarwasz @chtompki @struberg ?

One big difference is that the current implementation does not allocate any extra memory. I wonder what happens to performance of existing apps that call these methods on large strings.

[garydgregory]

I have rebased onto master and have followed your comments.

Hi @IBue

It looks like you forgot to run mvn (by itself) before pushing, see the checkstyle error in StringUtils.

[IBue]

note on performance:
boxing is the main responsible for increase in memory footprint here, that is +3GB for a 100M input sequence,
but the current implementation takes already a 10² order of magnitude more runtime than the proposed one for a 10M input sequence (minutes!) and a result index at half of the input size.

[ppkarwasz]

@IBue,

note on performance: boxing is the main responsible for increase in memory footprint here, that is +3GB for a 100M input sequence, but the current implementation takes already a 10² order of magnitude more runtime than the proposed one for a 10M input sequence (minutes!) and a result index at half of the input size.

Can you tell us more on the usage you have for this method? That will tell us a lot on how it should be optimized.

A common usage for this method could be extracting contiguous series of digits from a long string. With series of 10-20 digits it doesn't really make sense to go parallel, but if you want to validate a 100M input to make sure it only contains digits, you can sacrifice more memory.

Note: The indexOfAnyBut method can have multiple implementations and switched between them based on the input size.

[garydgregory]

@IBue
Please reply to @ppkarwasz's question:

Can you tell us more on the usage you have for this method? That will tell us a lot on how it should be optimized.

[IBue]

I have transformed the ListIterator loop into the proposed simplified index-based one.
Does anyone know if the index has to be protected against overflow?

Can you tell us more on the usage you have for this method? That will tell us a lot on how it should be optimized.

Actually, I don't have productive usage for this (yet). I stumbled upon the problem in the context of UTF-16 and learning some Java.

[ppkarwasz]

@IBue,

This looks good to me.

[garydgregory]

@IBue @ppkarwasz
Another issue here is that for every single step in the for loop, the PR does a sequential search in a collection. Is it worth doing a binary search instead (at the cost for sorting the array)? Like this:

    public static int indexOfAnyBut(final CharSequence seq, final CharSequence searchChars) {
        if (isEmpty(seq) || isEmpty(searchChars)) {
            return INDEX_NOT_FOUND;
        }
        final int[] codePoints = searchChars.codePoints().sorted().toArray();
        // advance character index from one interpreted codepoint to the next
        for (int curSeqCharIdx = 0; curSeqCharIdx < seq.length();) {
            final int curSeqCodePoint = Character.codePointAt(seq, curSeqCharIdx);
            if (Arrays.binarySearch(codePoints, curSeqCodePoint) < 0) {
                return curSeqCharIdx;
            }
            curSeqCharIdx += Character.charCount(curSeqCodePoint); // skip indices to paired low-surrogates
        }  
        return INDEX_NOT_FOUND;
    }

[ppkarwasz]

@garydgregory,

Another issue here is that for every single step in the for loop, the PR does a sequential search in a collection. Is it worth doing a binary search instead (at the cost for sorting the array)? Like this:

Currently it is a lookup in a Set (not a generic collection):

commons-lang/src/main/java/org/apache/commons/lang3/StringUtils.java

Lines 2877 to 2887 in e0e3f08

	final Set<Integer> searchSetCodePoints = searchChars.codePoints()
	.boxed().collect(Collectors.toSet()); // JDK >=10: Collectors::toUnmodifiableSet
	// advance character index from one interpreted codepoint to the next
	for (int curSeqCharIdx = 0; curSeqCharIdx < seq.length();) {
	final int curSeqCodePoint = Character.codePointAt(seq, curSeqCharIdx);
	if (!searchSetCodePoints.contains(curSeqCodePoint)) {
	return curSeqCharIdx;
	}
	curSeqCharIdx += Character.charCount(curSeqCodePoint); // skip indices to paired low-surrogates
	}
	return INDEX_NOT_FOUND;

[garydgregory]

@IBue Would you please rebase on git master?

[IBue]

I have parameterized the test functions and rebased onto master.

(You made premature, conflicting changes without proper comment in 1a89cca )

[ppkarwasz]

LGTM

[garydgregory]

@IBue @ppkarwasz
Merged! TY for the work. Do you see where else Lang might have issues with UTF-16 surrogates?

[IBue]

Thank you for reviewing and valuable feedback!
I am preparing the transfer to StringUtils:indexOfAny; and I think there are still quite a few functions that need reconsideration regarding UTF-16 correctness and scalability.

[garydgregory]

@IBue
Let us know what you find 👀