Avoid quadratic canonical combining-class ordering - #348
Conversation
|
Wouldn't it just be simpler to just call (Alternatively, since there are only a few combining classes, a radix sort might be more efficient?) I'm also not thrilled about how verbose this code is. |
|
Thanks — radix is a better fit here. I switched the long-run path to an allocation-free stable LSD radix sort and cut the helper code substantially. I didn't use qsort because equal combining classes need to retain their input order, while C qsort isn't stable. The updated version is byte-identical to current master on the randomized NFD/NFC/CHARBOUND differential corpus, passes 9/9 tests, and the 256 KB adversarial case is ~0.020 s vs. ~10.928 s on current master. |
| return unsafe_get_property(uc)->combining_class; | ||
| } | ||
|
|
||
| static void canonical_order_reverse(utf8proc_int32_t *buffer, utf8proc_ssize_t first, utf8proc_ssize_t last) { |
There was a problem hiding this comment.
These algorithms, although they look simple, have non-trivial pre- and post-conditions. I would add a few comments describing these, and also describing the loop invariants. Since the function arguments are used as indices into arrays, respective constraints should also be described.
There is a variable last here, but buffer[last] is never accessed. Presumably it's the last-plus-one index – maybe choose a different name? C++ calls it end.
| } | ||
| } | ||
|
|
||
| static void canonical_order(utf8proc_int32_t *buffer, utf8proc_ssize_t length) { |
There was a problem hiding this comment.
Switch to a more descriptive function name? Of course, rearrange_into_canonical_order is a bit long. Maybe order_canonically?
| } | ||
|
|
||
| static void canonical_order_reverse(utf8proc_int32_t *buffer, utf8proc_ssize_t first, utf8proc_ssize_t last) { | ||
| while (first < --last) { |
There was a problem hiding this comment.
I think this is a generic reverse function, and the canonical_order_ is just a prefix to avoid name collisions. This confused me at first, I thought this would produce a reverse canonical order.
There was a problem hiding this comment.
Maybe a comment in front of the function would already be enough.
There was a problem hiding this comment.
I would just rename it to reverse_buffer or something like that.
3 participants
Summary
Avoid quadratic work when canonical ordering processes a long run of combining characters.
utf8proc_decompose_custom()currently restores canonical combining-class order with adjacent swaps and backtracking. Because valid Unicode can contain an unbounded number of combining marks after a starter, a run with many class inversions makes this loop quadratic in the number of marks.This keeps short combining runs on the existing-style stable insertion path and uses a stable allocation-free LSD radix sort for long runs. The radix path partitions on the eight bits of the canonical combining class and preserves the relative order of equal-class marks.
The ordering does not cross CCC-0 starters or internal CHARBOUND markers.
Reproduction / performance
On current
master(0075ed7d0adba45682ee6bf7a83b10f8fd110163), I benchmarked valid UTF-8 consisting of one starter followed by repeated U+0301 (CCC 230) / U+0327 (CCC 202) pairs, built with GCC 15.2-O2:The current timings approach 4x CPU when input doubles at the larger sizes. The candidate removes that amplification without adding heap allocation to the lower-level decomposition API.
For ordinary short, already ordered combining runs, repeated 1 MB benchmarks remain in the same range (0.535 s baseline vs. 0.587 s patched for 20 rounds in the latest comparison).
Correctness validation
NormalizationTest.txtandGraphemeBreakTest.txt.0075ed7don a deterministic 600 KB corpus of randomized starters and combining classes 1, 202, 220, 230, 232, and 240.-Wall -Wextra -Wpedanticbuild andgit diff --checkare clean.Security relevance
This is algorithmic-complexity hardening for applications that normalize attacker-controlled text. The input used above is valid Unicode and requires only a few hundred KB to consume multiple seconds of CPU on the current implementation. I am not claiming code execution or a universal remotely reachable DoS; reachability depends on how a consumer exposes normalization.
Related discussion in #298 concerns combining-run handling for a proposed normalized-equality API. This change targets the existing canonical-ordering path used by
utf8proc_map/utf8proc_decompose_custom.