sunchao commented on code in PR #6604:
URL: https://github.com/apache/datafusion-comet/pull/6604#discussion_r4178477844
##########
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:
[P2] Bound the ASCII pre-scan for long mixed UTF-8 strings. For a valid
4096-byte value containing 4094 ASCII bytes followed by `é`, `is_ascii()` scans
nearly the entire value before `decode_non_ascii_utf8_spark_lossy()` validates
it again with `str::from_utf8`. With repeated values and the default
`spark.comet.shuffle.jvm.preferDictionary.ratio=10`, two independent runs of
`append_columns` plus `builder_to_array` measured 422.62→489.68 and
406.41→455.81 ns/row, a 12–16% slowdown versus base. Output remains correct,
but this adds measurable CPU overhead to an existing JVM shuffle path. Could we
restrict the pre-scan to short values or otherwise avoid rescanning the prefix?
Evidence: Reproduced on x86_64 AMD EPYC-Milan with rustc 1.98.1, Arrow
59.3.0, release optimization, thin LTO, one codegen unit, and `-C
target-cpu=native`. Verified the probe's base/head conversion and decoder
sources against the exact requested Git revisions. Only dependency plumbing and
visibility were adapted, and serialization was excluded. Input: 8192 separately
allocated aligned UnsafeRows, identical 4096-byte values, every tenth row null.
Each result is the median of 11 alternating base/head samples. Run:
`CARGO_BUILD_JOBS=3 RUSTFLAGS='-C target-cpu=native' cargo run --release
--offline --manifest-path /tmp/comet-6604-review/conversion-probe/Cargo.toml`.
Fresh logs: `/tmp/comet-6604-review-current-benchmark.log` and
`/tmp/comet-6604-review-current-benchmark-repeat.log`. ASCII-only controls
improved in both runs.
--
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]
