Doris-Breakwater commented on issue #66439:
URL: https://github.com/apache/doris/issues/66439#issuecomment-5177692500
## Initial assessment
**Verdict: valid and actionable.** The reported lock-contention mechanism is
confirmed in the current source, and the proposed explicit ICU root-locale fix
is minimal and technically appropriate. The reported 82.44% CPU share is
incident evidence rather than something independently reproduced here, so the
mechanism is verified while the exact performance magnitude remains
workload-specific.
The issue is currently open with no labels and no prior comments. Suggested
triage labels are `kind/fix`, `kind/performance`, `area/sql/function`, and
`kind/behavior-changed`. A matching fix is already open as #66440.
### Verified facts
- Current upstream `master` (`cd0efa38f7224acbe83ebee03b99535fdfb3b6dc`)
still calls no-argument `UnicodeString::toUpper()` / `toLower()` in:
- `TransferImpl::to_upper_utf8`
- `TransferImpl::to_lower_utf8`
- the lowercase stage of `InitcapImpl::to_initcap_utf8`
- Doris currently pins ICU `release-69-1`, matching the `_69` symbols in the
flame graph.
- In ICU 69.1, the no-argument overloads pass a null locale to
`ustrcase_getCaseLocale()`. That function calls `uloc_getDefault()`, which
reaches `locale_get_default()` / `Locale::getDefault()` and acquires
`gDefaultLocaleMutex`.
- With `icu::Locale::getRoot()`, ICU receives an empty root-locale base name
and selects `UCASE_LOC_ROOT` without consulting the process default locale.
This removes the per-call default-locale mutex from these case-conversion paths
after normal one-time locale-cache initialization.
- Doris chooses the ASCII versus ICU implementation for the entire
`ColumnString` buffer. If any value in a block is non-ASCII, every row in that
block is processed by the ICU loop, including otherwise-ASCII values.
Consequently, the current code can perform the default-locale lookup once per
row and amplify contention across scanner threads.
- Locale-dependent output is also real on that ICU path. For example, a
Turkish default locale changes the mappings of ASCII `I`/`i`; using the root
locale makes results independent of the BE process environment. Existing tests
already encode language-neutral expectations such as `LOWER('KIZILAY') =
'kizilay'`, which supports root-locale semantics for these SQL functions.
- The all-ASCII SIMD path is not affected by this change.
### Evidence still needed
No additional information is required to establish the source-level call
chain or to proceed with review of the proposed fix. To make the reported
performance improvement reproducible and suitable for a release claim, please
add:
1. The affected Doris version or exact commit SHA, build type, and whether
the deployed BE used Doris's bundled ICU 69.1 or another ICU build.
2. A minimal concurrent reproduction or benchmark, including thread/scanner
concurrency, row count, block size, average string length, and the
proportion/distribution of non-ASCII values.
3. Comparable before/after profiles from the same host, data, query, and
concurrency, plus wall time, CPU time, throughput, and result checksum. The
expected post-fix signal is that `locale_get_default_69` and its spinlock
disappear from beneath these conversion functions.
4. If the production query itself is used for validation, the relevant Doris
query profile and query/session variables, with sensitive identifiers removed.
### Recommended next steps
1. Review #66440 as the candidate fix. Its three production call-site
changes match the verified root cause.
2. Require the locale-independence BE test to exercise the ICU branch under
a Turkish ICU default for `LOWER`, `UPPER`, and `INITCAP`; include
`LCASE`/`UCASE` aliases if maintainers want explicit alias coverage. The test
should always restore the process-wide ICU default locale.
3. Run the existing string-function BE unit tests and a concurrent
before/after benchmark. CI for #66440 is still pending at the time of this
analysis, so test completion should not yet be inferred.
4. Keep the behavior-change release note: deployments whose BE process
default is non-root may observe different Unicode case-mapping results, while
default/root-like deployments should remain unchanged.
5. After the master fix is accepted, inspect each maintained branch for the
same call sites before deciding on backports; do not assume branch equivalence.
Breakwater-GitHub-Analysis-Slot: slot_be95347985b2
--
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]
