Skip to content

Conversation

@ChrisDryden
Copy link
Collaborator

@ChrisDryden ChrisDryden commented Jan 2, 2026

This PR was originally made by @sylvestre back in sept to measure the performance that adding localy sorting would have. When I was investigating why there was such a slowdown I found that the benchmarks were actually not working correctly because the locale would be set to the first locale in the tests instead of to what the env variable was set to.

Now that the benchmarks are updated, it shows that the performance decline for the c locale is negligible and that the only declines are when using the locale env variables. This it not even a "decline" per se because these sorts were not even working in the first place.

@codspeed-hq
Copy link

codspeed-hq bot commented Jan 2, 2026

CodSpeed Performance Report

Merging #9995 will degrade performance by 94.31%

Comparing ChrisDryden:sort-perf-rebased (7531d74) with main (996e5e8)

Summary

❌ 7 regressions
✅ 132 untouched
⏩ 37 skipped1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Efficiency
sort_key_field[500000] 690.6 ms 1,090 ms -36.64%
sort_dictionary_order[500000] 1.1 s 1.1 s -3.81%
sort_german_de_locale 37.9 ms 665.9 ms -94.31%
sort_ascii_utf8_locale 20.1 ms 86 ms -76.65%
sort_reverse_utf8_locale 37.6 ms 609.2 ms -93.82%
sort_mixed_utf8_locale 37.4 ms 600.7 ms -93.77%
sort_unique_utf8_locale 37.7 ms 523.1 ms -92.8%

Footnotes

  1. 37 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.

@ChrisDryden ChrisDryden marked this pull request as ready for review January 2, 2026 21:36
@ChrisDryden
Copy link
Collaborator Author

@sylvestre how would you feel about merging your changes now that the benchmarks are corrected?

@sylvestre
Copy link
Contributor

sylvestre commented Jan 2, 2026

seems that https://github.com/uutils/coreutils/pull/9982/files#diff-e7bdf46e4a9e8a87200b91ae7a3eb7b406ac2d4388b70e108a7197ad93feaae9 is doing something similar for join
maybe we should add

pub fn should_use_locale_collation() -> bool {
    get_collating_locale().0 != DEFAULT_LOCALE
}

(or the equivalent) in uucore in a new pr

wdyt?

@ChrisDryden
Copy link
Collaborator Author

Yea that should definitely be in a shared place and have benchmarks to make sure that the more locale stuff we roll out does not impact performance. I'll make an example PR.

@sylvestre
Copy link
Contributor

OK to land but please fix the jobs

@github-actions
Copy link

github-actions bot commented Jan 5, 2026

GNU testsuite comparison:

GNU test failed: tests/sort/sort-rand. tests/sort/sort-rand is passing on 'main'. Maybe you have to rebase?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants