Repository navigation
wc: fix character counts for malformed UTF-8 input - #15139
darkraider01 wants to merge 12 commits into
Conversation
| let buf: &mut [u8] = &mut AlignedBuffer::default().data; | ||
| let policy = SimdPolicy::detect(); | ||
| let simd_allowed = wc_simd_allowed(policy); | ||
| let validate_utf8 = COUNT_CHARS && is_utf8_locale(); |
There was a problem hiding this comment.
This change only updates UTF-8 counting. The three C locale failures in wc/wc.pl remain and need a follow-up.
| let mut validate_with_simd = simd_allowed; | ||
| while !input.is_empty() { | ||
| let result = if validate_with_simd { | ||
| simdutf8::compat::from_utf8(input) |
There was a problem hiding this comment.
I added simdutf8 because scalar validation noticeably slowed down valid UTF-8 input. After the first invalid sequence in a chunk, validation switches to the standard library to avoid repeated SIMD setup.
|
GNU testsuite comparison: |
Merging this PR will regress 9 benchmarks
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | split_lines |
8.5 ms | 11.1 ms | -23.5% |
| ❌ | Simulation | split_numeric_suffix |
8.7 ms | 11.4 ms | -23.08% |
| ❌ | Simulation | wc_chars_large_line_count[100000] |
2.3 ms | 2.8 ms | -15.57% |
| ❌ | Simulation | five_38_bit_primes |
1.7 s | 1.9 s | -12.55% |
| ❌ | Simulation | tsort_complex_dag[50000] |
87.6 ms | 97.1 ms | -9.79% |
| ❌ | Simulation | tsort_tree_dag[(10, 3)] |
36.5 ms | 39.4 ms | -7.33% |
| ❌ | Simulation | sort_dictionary_order[500000] |
1.9 s | 2.1 s | -6.59% |
| ❌ | Simulation | tsort_wide_dag[100000] |
157.6 ms | 167 ms | -5.65% |
| ❌ | Simulation | tsort_linear_chain[1000000] |
1.9 s | 2 s | -4.75% |
| ⚡ | Simulation | three_39_bit_primes |
545.9 ms | 255.9 ms | ×2.1 |
| ⚡ | Simulation | check_sorted_utf8_locale |
491.1 ms | 422.9 ms | +16.13% |
| ⚡ | Simulation | sort_numeric_utf8_locale |
40.8 ms | 36.1 ms | +12.99% |
| ⚡ | Simulation | expand_many_lines[100000] |
106.2 ms | 95 ms | +11.77% |
| ⚡ | Simulation | merge_single_file_utf8_locale |
133.4 ms | 119.4 ms | +11.76% |
| ⚡ | Simulation | expand_custom_tabstops[50000] |
29.6 ms | 26.9 ms | +10.22% |
| ⚡ | Simulation | merge_pre_sorted_files_utf8_locale |
251.9 ms | 238 ms | +5.81% |
| ⚡ | Simulation | merge_pre_sorted_files |
252.5 ms | 238.9 ms | +5.71% |
| ⚡ | Simulation | sort_german_de_locale |
295 ms | 282.1 ms | +4.6% |
| ⚡ | Simulation | thirteen_39_bit_primes |
9.3 s | 8.9 s | +4.44% |
| ⚡ | Simulation | sort_case_sensitive[500000] |
334.9 ms | 321 ms | +4.32% |
| ... | ... | ... | ... | ... | ... |
ℹ️ Only the first 20 benchmarks are displayed. Go to the app to view all benchmarks.
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing darkraider01:wc-utf8-character-counts (3a107c7) with main (0d4a305)
Footnotes
-
54 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
|
||
| const BUF_SIZE: usize = 64 * 1024; | ||
|
|
||
| fn is_utf8_locale() -> bool { |
There was a problem hiding this comment.
please fix get_ctype_encoding() in uucore for locales without a suffix like en_IN instead of adding this unsafe locale code in wc
| } | ||
| None => return 0, | ||
| } | ||
| } |
There was a problem hiding this comment.
codspeed shows a 39% regression, could you please look at it?
the is_ascii() pass followed by from_utf8 reads non-ASCII chunks twice
There was a problem hiding this comment.
I looked into the CodSpeed report. The updated comparison shows about 18% lower efficiency for wc_chars_large_line_count; it also flags different runtime environments, so the exact size of the regression is uncertain. I'll look into it further.
Locally, removing is_ascii() slowed ASCII input, especially with SIMD disabled. A tail-first check helped some UTF-8 inputs but regressed others, so I reverted it. These experiments haven’t resolved the regression; the additional UTF-8 validation still adds work, and this remains open.
There was a problem hiding this comment.
Okay so i digged deeper into it and benchmarked the PR against its base on my machine, with the same compiler and locale, alternating runs pinned to one CPU. The affected ASCII benchmark measured 0.764 ms on the base and 0.772 ms on the PR, within the variation between runs, so I couldn’t reproduce the reported regression locally.
There is a clear cost for valid UTF-8: that benchmark was about 68% slower. I also tried a scalar pass that validates and counts together, but it was substantially slower than the current SIMD approach, so I dropped it.
I’ve kept the current implementation for now. The local results don’t explain the CodSpeed regression, so I’m leaving this open.
Would you prefer further performance work in this PR, or a separate follow-up?
| } | ||
|
|
||
| // Fast paths that can be computed without Unicode decoding. | ||
| // Fast paths for byte, character, and line counts. |
There was a problem hiding this comment.
I changed it because the character-counting path now validates UTF-8, so “without Unicode decoding” seemed less accurate. The new wording just describes which counts use these fast paths. Happy to keep the original comment if you prefer.
| for (key, val) in &cmd_env { | ||
| // WASI reads the first duplicate, whereas Command::envs uses the last. | ||
| // Resolve overrides before forwarding them to the guest. | ||
| let wasm_env: BTreeMap<_, _> = cmd_env.iter().cloned().collect(); |
There was a problem hiding this comment.
unrelated to wc, please move it into a separate PR
| rustix = { version = "1.1.4", default-features = false } | ||
| self_cell = "1.0.4" | ||
| selinux = "0.6" | ||
| simdutf8 = "0.1.5" |
There was a problem hiding this comment.
is std from_utf8 really that much slower here?
There was a problem hiding this comment.
I checked this directly on my machine, using the same UTF-8 benchmark and changing only the validator. With SIMD enabled, it took about 0.267 ms with simdutf8 and 1.325 ms with std::str::from_utf8 , roughly 5× slower across nine alternating runs. The counting code stayed the same, so I’ve kept simdutf8. This doesn’t explain the earlier CodSpeed ASCII regression, though.
| /// Return the character-type encoding (`LC_CTYPE`) deduced from the environment. | ||
| /// Return the character-type encoding (`LC_CTYPE`) selected by the environment. | ||
| /// Bare locale names are resolved through native locale data where available. | ||
| pub fn get_ctype_encoding() -> UEncoding { |
There was a problem hiding this comment.
could you please move the uucore change into its own PR? same as the test helper
There was a problem hiding this comment.
Moved it to #15199 and removed it from this PR. Bare-locale detection will depend on that landing first.
| *CTYPE_ENCODING.get_or_init(|| { | ||
| let name = ["LC_ALL", "LC_CTYPE", "LANG"] | ||
| .iter() | ||
| .find_map(|key| std::env::var(key).ok().filter(|value| !value.is_empty())); |
There was a problem hiding this comment.
this duplicates the LC_ALL/LC_CTYPE/LANG lookup of get_locale_from_env, could it be shared?
There was a problem hiding this comment.
Shared the lookup in #15199. Both paths now use the same environment lookup and parsing helpers, and get_ctype_encoding reads the environment only once.
| // SAFETY: locale is live while CODESET is read. The previous thread locale | ||
| // is restored before the owned locale is freed. | ||
| unsafe { | ||
| let previous = libc::uselocale(locale); |
There was a problem hiding this comment.
could nl_langinfo_l be used here instead of switching the thread locale back and forth?
There was a problem hiding this comment.
Yes, switched to nl_langinfo_l in #15199. It queries the locale object directly, so there’s no thread-locale switching anymore.
The fast path for
wc -mandwc -cmcounts some invalid or incomplete UTF-8 sequences as characters. This validates the input in UTF-8 locales and handles characters split across reads, with regression tests for those cases.Related to #14928