andygrove commented on code in PR #6604:
URL: https://github.com/apache/datafusion-comet/pull/6604#discussion_r4179581467
##########
native/common/src/utils.rs:
##########
@@ -52,7 +52,22 @@ pub fn bytes_to_i128(slice: &[u8]) -> i128 {
/// per-class malformed lengths below (E0/ED overlong & surrogate handling,
F0/F4 range checks)
/// match the observable replacement behavior of the JDK UTF-8 decoder; they
were determined from
/// observed `new String(bytes, UTF_8)` output, not by reviewing the OpenJDK
source.
+// Inlined so that callers in other crates, such as the JVM columnar shuffle
converting one value at
+// a time, get the ASCII check without a call.
+#[inline]
pub fn decode_utf8_spark_lossy(bytes: &[u8]) -> Cow<'_, str> {
+ // ASCII is valid UTF-8. `is_ascii` checks a word at a time, and on short
strings it is several
+ // times cheaper than `str::from_utf8`, whose cost dominated the JVM
columnar shuffle's string
+ // conversion.
+ if bytes.is_ascii() {
Review Comment:
Fixed in b3459f066d. Values of up to 32 bytes still get the inline
`is_ascii()` check. Longer values are checked out of line 32 bytes at a time,
and `str::from_utf8` validates only from the first block that isn't ASCII, so
at most 32 bytes are read twice.
It was worse on aarch64 than in your x86 run. There std's `is_ascii` tests
one word at a time and is slower than `str::from_utf8` on long values, so long
pure-ASCII values had regressed too. Per-row cost of `append_columns` plus
`builder_to_array` on an M3 Max, best of four interleaved runs, 8192 rows with
every tenth null (ratio is `preferDictionary.ratio`):
| Column | main | b478aa7f28 | b3459f066d |
| --- | --- | --- | --- |
| 4094 ASCII bytes then `é`, ratio 10 | 368 | 505 | 336 |
| 4094 ASCII bytes then `é`, ratio 0 | 315 | 440 | 289 |
| 4096 ASCII bytes, ratio 10 | 363 | 420 | 327 |
| 98 ASCII bytes then `é`, ratio 0 | 15.8 | 15.2 | 12.8 |
| 63 bytes of CJK, ratio 0 | 37.0 | 37.5 | 38.0 |
| `value-N` (7-12 bytes), ratio 0 | 12.2 | 5.6 | 5.4 |
The block check is out of line because inlining it stopped `get_string` from
inlining into the shuffle's row loops, which cost short strings almost 2 ns.
It's `#[cold]` because otherwise LLVM put an extra jump on the short ASCII
path, another 0.7 ns. The CJK row is the price of a failed block check on a
value over 32 bytes, about 1 ns.
I don't have an x86 machine to rerun your probe on. With
`-Ctarget-cpu=x86-64-v3`, which the release builds use, the block check is six
instructions with one branch per 32 bytes.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
