[emirmrkaljevic]

Fixes #15615

• check.rs: keep the reader thread's JoinHandle and return its error after draining, so sort -c/-C on an unreadable input exits 2 with read failed: ... instead of reporting the partial input as sorted. A disorder found before the error still takes precedence.
• check.rs and merge.rs: call setup_localization("sort") at the start of the reader threads, since localization is per thread and the read error message is built there (sort -m input dir printed sort: sort-read-failed). Same as what dd does in its progress thread.

Added test_read_error_on_reader_thread with -c dir, -C dir and -m input dir; all three fail on main and pass with this change. It only checks for read failed: and Is a directory, so it won't clash with #15191 adding the file name. test_sort:: passes locally except test_merge_batch_size_with_limit and test_temporary_files_with_few_file_descriptors, which fail on main too on my machine (fd limit). Clippy is clean.

Found with behave.

[codspeed]

Merging this PR will not alter performance

1 warning, 1 note

⚠️ 117 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

ℹ️ 1 benchmark has no flamegraph

The flamegraph wasn't generated: most of the time was spent in runtime internals, which would leave nothing useful to analyze.

See affected benchmarks

✅ 401 untouched benchmarks
⏩ 70 skipped benchmarks¹

---

_{Comparing emirmrkaljevic:sort-reader-thread-errors (b73553f) with main (8b87ab5)}

Footnotes

1. 70 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. ↩

[sylvestre]

could chunks.rs return a SortError variant instead? its Display is translated lazily on the main thread, so we wouldn't need the setup in every reader thread

[sylvestre]

did you run the GNU sort tests with this change?
bash util/build-gnu.sh && bash util/run-gnu-test.sh tests/sort/sort-merge.pl tests/sort/sort.pl

[sylvestre]

this history comment isn't really needed, the test name says enough

[sylvestre]

a bunch of jobs are failing

[emirmrkaljevic]

Thanks for the review! Addressed in b73553f:

• chunks::read now returns SortError::ChunkReadFailed instead of USimpleError, so Display is translated lazily on the main thread and setup_localization is no longer needed in check.rs or merge.rs (this also fixes the CodSpeed memory regression on the merge benchmarks).
• Removed the history comment in test_read_error_on_reader_thread.
• Ran the GNU sort tests (tests/sort/sort-merge.pl and tests/sort/sort.pl): both pass (TOTAL: 2, PASS: 2, FAIL: 0).

[sylvestre]

@codspeedbot fix this regression

[codspeed]

@codspeedbot fix this regression

The latest commit, b73553f, should already fix this regression, so I didn't push any changes.

Cause: On 3784e86, the memory flamegraph for merge_single_file_utf8_locale shows uucore::locale::setup_localization running inside the uu_sort::merge::reader thread. That call builds a separate Fluent bundle for the thread, and it accounts for the extra ~16 KB / ~89 KB peak memory in merge_single_file_utf8_locale and merge_pre_sorted_files.

Fix (b73553f): That commit removes setup_localization from the merge.rs and check.rs reader threads. The read error is now returned as SortError::ChunkReadFailed and only translated when it's displayed on the main thread. With this change:

• src/uu/sort/src/merge.rs is byte-identical to main.
• The only change in chunks.rs is in the error branch, which these benchmarks never reach.

So the merge benchmarks run the same code as on main and should go back to baseline memory.

I couldn't run the memory benchmarks myself in this session. CodSpeed's CI run for b73553f was still processing when I finished, so its report on this PR will be the confirmation. If either benchmark still shows a regression there, comment @codspeedbot investigate the remaining regression and I'll look into it.

Summary

• Warming up the performance wizard...
• Identify the cause of the memory regression — The extra allocations come from setup_localization running in the merge reader thread (it loads a Fluent bundle for each thread). b73553f already removes that call.
• Measure memory before and after the fix — Could not measure memory in this session. Checked the code instead: merge.rs is now byte-identical to main. The CodSpeed run for b73553f is still processing.
• Handling comment — Comment handling finished successfully

_{Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.}

[github-actions]

Binary size comparison:

Individual binary size comparison VS main (threshold: >=5% AND >=4 KB).

Total size of compared binaries: 155.04 MB (-308 KB, -0.19%)

Significant per-binary changes:
  date     2.61 MB ->    2.30 MB  (-316 KB, -11.81%)

[sylvestre]

Thanks for your PR