draft: Port GetPointerToFirstInvalidByte from SimdUnicode - #974

@BenjaminBucher

Thanks for the PR. What about making the function more generic ?

Here is a gist (not a proposal)...

struct utf8_segment {
  size_t length; // length in bytes
  size_t ascii; // number of ASCII characters
  size_t two_bytes; // number of two-byte (UTF-8) characters
  size_t three_bytes; // number of three-byte (UTF-8) characters
  size_t four_bytes; // number of four-byte (UTF-8) characters
}

And then we have something like...

utf8_segment  find_first_invalid_utf8(const char * input, size_t length);

This would be a great building block that could be used for various applications, not just UTF-8 to UTF-16 transcoding.

cc @pauldreik @anonrig @erikcorry

I'm definitely in favor of making this function more generic.
The concrete error code could also be returned by the function, as it is quite cheap to compute if we already compute the error location (and might also be helpful).

@BenjaminBucher Yeah.

What we want is the common case to be fast (which is going to be proper UTF-8).

I'm highly confused by the current compile error:

In file included from /src/src/haswell/implementation.cpp:103,
                 from /src/src/simdutf.cpp:133:
/src/src/generic/ascii_validation.h: In function 'simdutf::result simdutf::simdutf::haswell::{anonymous}::ascii_validation::generic_validate_ascii_with_errors(const char*, size_t)':
/src/src/generic/ascii_validation.h:7:3: error: 'buf_block_reader' was not declared in this scope; did you mean 'simdutf::haswell::{anonymous}::buf_block_reader'?
    7 |   buf_block_reader<64> reader(reinterpret_cast<const uint8_t *>(input), length);
      |   ^~~~~~~~~~~~~~~~
      |   simdutf::haswell::{anonymous}::buf_block_reader

It complains about buf_block_reader not being available in ascii_validation.h.
I do not understand which of my changes might've broken the compilation of that file. buf_block_reader should be perfectly available and is used in the same way in utf8_validator.h, which is a file I did modify.

I'm highly confused by the current compile error:

In file included from /src/src/haswell/implementation.cpp:103,
                 from /src/src/simdutf.cpp:133:
/src/src/generic/ascii_validation.h: In function 'simdutf::result simdutf::simdutf::haswell::{anonymous}::ascii_validation::generic_validate_ascii_with_errors(const char*, size_t)':
/src/src/generic/ascii_validation.h:7:3: error: 'buf_block_reader' was not declared in this scope; did you mean 'simdutf::haswell::{anonymous}::buf_block_reader'?
    7 |   buf_block_reader<64> reader(reinterpret_cast<const uint8_t *>(input), length);
      |   ^~~~~~~~~~~~~~~~
      |   simdutf::haswell::{anonymous}::buf_block_reader

It complains about buf_block_reader not being available in ascii_validation.h. I do not understand which of my changes might've broken the compilation of that file. buf_block_reader should be perfectly available and is used in the same way in utf8_validator.h, which is a file I did modify.

sometimes it is helpful to do an amalgamation, and work with the amalgamated source. the include system can be very confusing.

@pauldreik

sometimes it is helpful to do an amalgamation, and work with the amalgamated source. the include system can be very confusing.

Ah Ah

I think it is quite simple and a classic: a missing final brace (}) which makes the C++ compiler go wild and report strange errors.

@pauldreik Yes, the include system did confuse me a couple of times already, but I'm slowly getting a feel for it. Thanks for the tip with the amalgamation.
I also kind of gave up using an IDE for now an am just working with the compiler messages the classic way :)

another tip for detecting silly mistakes like a missing brace is to run the clang format script: you often get large changes in the wrong place.

see scripts/clang_format.sh

@lemire
I fear that the linear system of equations to calculate number of ascii symbols and 2-/3-byte leads with the current set of counters is underdetermined.

We got: length (amount of bytes), ncon (amount of continuation bytes), n4 (amount of 4-byte leads)
We want: n1, n2, n3 (amount of ascii bytes, 2-/3-byte leads)

length = n4 * 4 + n3 * 3 + n2 * 2 + n1
ncon = n4 * 3 + n3 * 2 + n2

If I didn't miss anything, we only have 2 equations.
To calculate the utf16 length we can luckily do without solving the linear equation.
However, as we have fewer equations than unknowns, we can't provide all these numbers with just keeping track of the amount of continuation bytes and 4-byte leads.

We'd need to keep track of another variable, which we can do, but will add more runtime overhead not required for utf16 length.
This might not be too bad, but a relevant decision, which is why I want input :)

@BenjaminBucher You are, of course, correct.

I think we need at least one more piece of information if we want to proceed with my sketch of an idea: the total number of ASCII bytes ($n_1$).

Let (L) be the total number of bytes in the string. We define:

• $n_1$: number of 1-byte characters (ASCII)
• $n_2$: number of 2-byte characters
• $n_3$: number of 3-byte characters
• $n_4$: number of 4-byte characters
• $n_{\rm con}$: number of continuation bytes

We have two expressions for the total byte length $L$:

$$ L = 4n_4 + 3n_3 + 2n_2 + n_1 $$

$$ L = n_4 + n_3 + n_2 + n_{\rm con} + n_1 $$

Solving the linear system for $n_3$ and $n_2$:

$$ n_3 = n_1 - 2n_4 + 2n_{\rm con} - L $$

$$ n_2 = -2n_1 + n_4 - 3n_{\rm con} + 2L $$

You might object that this will entail extra processing. It is a slight additional cost.

Right.

So maybe we can do something else instead.

What about this:

struct utf8_segment {
  size_t length; // length in bytes
  size_t characters; // number of characters ASCII, two-byte, three-byte and 4-byte
  size_t four_bytes; // number of four-byte (UTF-8) characters
}

The number of characters is:

$$ N = n_1 + n_2 + n_3 + n_4 = L - n_{\rm con} $$

That's it. Characters = bytes minus continuation bytes.

This generic data structure is enough to compute the required UTF-16 output (it is just characters + four-bytes). But also the required UTF-32 output (it is just characters).

The benefit of my approach, I think, it is that it is easier to debug.

The downside is that the data structure is slightly fatter than what is needed, if we just want to go to UTF-16, then we have slightly too much information. So there is a tiny performance penalty here, with my proposal, but I think that it is so small as to be irrelevant in practice.

With the focus on throughput, I first definitely recoiled from the idea of keeping yet another counter.
However, I am easily convinced that the actual impact can be negligible.

Keeping track of ascii/non-ascii characters sounds simple enough.

I'll adjust the generic implementation. In general, it could be slightly improved by duplicating code more code and integrating the counters deeper in the simd functions.
Next I'll take a look at the avx implementation :)

@BenjaminBucher I am not arguing that we need to keep track of the ASCII characters. It may indeed be quite cheap.

Fair fair, that sounds to me like a good solution :)

Among other things the compilers appear to not particularly like the tuples. They aren't that great anyways and should probably be replaced with a struct. Can I define the struct somewhere where it won't appear in the public API?

@BenjaminBucher I think you are doing fine, the errors look like details we can work out later.

This being said, I agree that a dedicated struct is better. Why not make it public? There is nothing wrong with public structures?

Fair point. I'm typically in favor of keeping the public interface minimal, and I wasn't convinced that the benefits of a struct for this are sufficient :)

The icelake implementation is embarrassingly simple and I want to propose to do the same in the generic implementation.
In the generic implementation I tried to stay close to the simdunicode implementation and because of that dug relatively deep into the simd validation code. I still tried to keep the validation logic somewhat separate to counts, in order to keep the logic simple and easier to verify.

For the icelake implementation I didn't check the simdunicode implementation and found it significantly easier to keep the counts out of the validator and instead do the counts where the reader is processed, chunk for chunk.

Trade-offs (non-exhaustive):

• Doing the counts alongside the validation logic would save ~2 avx-512 comparison operations, maybe another load but for that I'd need to look into the assembly. However, this would require us to duplicate the entire validation code and changing it in a few places with more state, which would make the code more complex.
• In the generic code, one reader chunk contains either 2 or 4 full simd registers worth of data. The count logic is currently inserted in a location where each of the simd register parts of the chunk is loaded only once. (Once again, at the cost of more code complexity).

I don't have good intuition for where overoptimization starts and where good-enough and simple code is in order. Feedback wanted :)

I’ll get back to you in the next few days.

@BenjaminBucher Sorry for the delay.

Ok. At this stage, we don't care about performance very much, at least not down to the details of each implementation. What we care a great deal is to get a correct results.

@BenjaminBucher The AVX-512 is not quite correct, see my comment. Once this is fixed, then I expect your code to be correct.

The next step would then be to wire in this function into our benchmarks, and, then, once we have benchmarks, it is easier to take decisions regarding the code.

As a rule, have slightly more complicated code for better performance is a good tradeoff in simdutf. Our main selling point is performance, not code simplicity.

I can hardly call a single day a delay ^^.
Where is your comment on the avx512 code, I'm not sure whether it was sent.
I'll take a look at benchmarking.

@BenjaminBucher You should now see my comments. They were in the system but not yet visible.

^ rebased onto master to fix conflicts